{"thread":{"id":"32723","subject":"[PATCH 3/7] git-mergetool: don't hardcode 'mergetool' in show_tool_help","startedAt":"2013-01-25T09:43:47Z","lastAt":"2013-01-26T13:50:23Z","messageCount":30,"participants":["David Aguilar","Sebastian Schuberth","John Keeping","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":7},"messages":[{"id":"207788","messageId":"1359107034-14606-1-git-send-email-davvid@gmail.com","threadId":"32723","inReplyTo":null,"subject":"[PATCH 0/7] mergetool-lib improvements for --tool-help","fromName":"David Aguilar","fromEmail":"davvid@gmail.com","sentAt":"2013-01-25T09:43:47Z","receivedAt":"2013-01-25T09:43:47Z","isPatch":true,"sender":{"key":"davvid@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13196?v=4"},"body":"I ran with John's idea and simplified a few more things so\nthat the --tool-help output shows the correct results.\n\nMy final commit is dependent on John's commits but the other two\ncould be picked up independently since they are general improvements.\n\nThis does add a few symlinks to the repo for the sake of simplicity;\nI'm not sure if this presents a problem for folks on other platforms.\n\nThe replacement of vim with vimdiff and symlinks simplifies things\nso that we can later auto-generate the list-of-all-tools for use\nby Documentation/.  This was not previously possible due to the\nspecial-case for the vim tools which this series eliminates.\n\nDavid Aguilar (3):\n  mergetools/vim: Remove redundant diff command\n  mergetools: Fix difftool/mergetool --tool-help listing for vim\n  mergetool--lib: Improve show_tool_help() output\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           | 55 +++++----------------------------------\n git-mergetool--lib.sh       | 63 +++++++++++++++++++++++++++++++++++++++------\n git-mergetool.sh            | 37 --------------------------\n mergetools/gvimdiff         |  1 +\n mergetools/gvimdiff2        |  1 +\n mergetools/{vim => vimdiff} | 12 ++-------\n mergetools/vimdiff2         |  1 +\n 7 files changed, 67 insertions(+), 103 deletions(-)\n create mode 120000 mergetools/gvimdiff\n create mode 120000 mergetools/gvimdiff2\n rename mergetools/{vim => vimdiff} (68%)\n create mode 120000 mergetools/vimdiff2\n\n-- \n1.8.1.1.367.g22b1720.dirty\n"},{"id":"207789","messageId":"1359107034-14606-2-git-send-email-davvid@gmail.com","threadId":"32723","inReplyTo":"1359107034-14606-1-git-send-email-davvid@gmail.com","subject":"[PATCH 1/7] git-mergetool: move show_tool_help to mergetool--lib","fromName":"David Aguilar","fromEmail":"davvid@gmail.com","sentAt":"2013-01-25T09:43:48Z","receivedAt":"2013-01-25T09:43:48Z","isPatch":true,"sender":{"key":"davvid@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13196?v=4"},"body":"From: John Keeping <john@keeping.me.uk>\n\nThis 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>\nSigned-off-by: David Aguilar <davvid@gmail.com>\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.1.367.g22b1720.dirty\n"},{"id":"207790","messageId":"1359107034-14606-3-git-send-email-davvid@gmail.com","threadId":"32723","inReplyTo":"1359107034-14606-1-git-send-email-davvid@gmail.com","subject":"[PATCH 2/7] git-mergetool: remove redundant assignment","fromName":"David Aguilar","fromEmail":"davvid@gmail.com","sentAt":"2013-01-25T09:43:49Z","receivedAt":"2013-01-25T09:43:49Z","isPatch":true,"sender":{"key":"davvid@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13196?v=4"},"body":"From: John Keeping <john@keeping.me.uk>\n\nTOOL_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>\nSigned-off-by: David Aguilar <davvid@gmail.com>\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.1.367.g22b1720.dirty\n"},{"id":"207783","messageId":"1359107034-14606-4-git-send-email-davvid@gmail.com","threadId":"32723","inReplyTo":"1359107034-14606-1-git-send-email-davvid@gmail.com","subject":"[PATCH 3/7] git-mergetool: don't hardcode 'mergetool' in show_tool_help","fromName":"David Aguilar","fromEmail":"davvid@gmail.com","sentAt":"2013-01-25T09:43:50Z","receivedAt":"2013-01-25T09:43:50Z","isPatch":true,"sender":{"key":"davvid@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13196?v=4"},"body":"From: John Keeping <john@keeping.me.uk>\n\nWhen 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>\nSigned-off-by: David Aguilar <davvid@gmail.com>\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.1.367.g22b1720.dirty\n"},{"id":"207787","messageId":"1359107034-14606-5-git-send-email-davvid@gmail.com","threadId":"32723","inReplyTo":"1359107034-14606-1-git-send-email-davvid@gmail.com","subject":"[PATCH 4/7] git-difftool: use git-mergetool--lib for \"--tool-help\"","fromName":"David Aguilar","fromEmail":"davvid@gmail.com","sentAt":"2013-01-25T09:43:51Z","receivedAt":"2013-01-25T09:43:51Z","isPatch":true,"sender":{"key":"davvid@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13196?v=4"},"body":"From: John Keeping <john@keeping.me.uk>\n\nThe \"--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>\nSigned-off-by: David Aguilar <davvid@gmail.com>\n---\n git-difftool.perl | 55 +++++++------------------------------------------------\n 1 file changed, 7 insertions(+), 48 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+\tmy $cmd = 'TOOL_MODE=diff';\n+\t$cmd .= ' && . \"$(git --exec-path)/git-mergetool--lib\"';\n+\t$cmd .= ' && show_tool_help';\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+\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.1.367.g22b1720.dirty\n"},{"id":"207785","messageId":"1359107034-14606-6-git-send-email-davvid@gmail.com","threadId":"32723","inReplyTo":"1359107034-14606-1-git-send-email-davvid@gmail.com","subject":"[PATCH 5/7] mergetools/vim: Remove redundant diff command","fromName":"David Aguilar","fromEmail":"davvid@gmail.com","sentAt":"2013-01-25T09:43:52Z","receivedAt":"2013-01-25T09:43:52Z","isPatch":true,"sender":{"key":"davvid@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13196?v=4"},"body":"vimdiff and vimdiff2 differ only by their merge command so remove the\nlogic in the diff command since it's not actually needed.\n\nSigned-off-by: David Aguilar <davvid@gmail.com>\n---\n mergetools/vim | 12 ++----------\n 1 file changed, 2 insertions(+), 10 deletions(-)\n\ndiff --git a/mergetools/vim b/mergetools/vim\nindex 619594a..39d0327 100644\n--- a/mergetools/vim\n+++ b/mergetools/vim\n@@ -1,14 +1,6 @@\n diff_cmd () {\n-\tcase \"$1\" in\n-\tgvimdiff|vimdiff)\n-\t\t\"$merge_tool_path\" -R -f -d \\\n-\t\t\t-c 'wincmd l' -c 'cd $GIT_PREFIX' \"$LOCAL\" \"$REMOTE\"\n-\t\t;;\n-\tgvimdiff2|vimdiff2)\n-\t\t\"$merge_tool_path\" -R -f -d \\\n-\t\t\t-c 'wincmd l' -c 'cd $GIT_PREFIX' \"$LOCAL\" \"$REMOTE\"\n-\t\t;;\n-\tesac\n+\t\"$merge_tool_path\" -R -f -d \\\n+\t\t-c 'wincmd l' -c 'cd $GIT_PREFIX' \"$LOCAL\" \"$REMOTE\"\n }\n \n merge_cmd () {\n-- \n1.8.1.1.367.g22b1720.dirty\n"},{"id":"207786","messageId":"1359107034-14606-7-git-send-email-davvid@gmail.com","threadId":"32723","inReplyTo":"1359107034-14606-1-git-send-email-davvid@gmail.com","subject":"[PATCH 6/7] mergetools: Fix difftool/mergetool --tool-help listing for vim","fromName":"David Aguilar","fromEmail":"davvid@gmail.com","sentAt":"2013-01-25T09:43:53Z","receivedAt":"2013-01-25T09:43:53Z","isPatch":true,"sender":{"key":"davvid@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13196?v=4"},"body":"\"git difftool --tool-help\" and \"git mergetool --tool-help\" incorreclty\nlist \"vim\" as being an unavailable tool.  This is because they attempt\nto find a tool named according to the mergetool scriptlet instead of the Git-\nrecognized tool name.\n\nvimdiff, vimdiff2, gvimdiff, and gvimdiff2 are all provided by the \"vim\"\nscriptlet.  This required git-mergetool--lib to special-case it when\nsetting up the tool.\n\nRemove the exception for \"vim\" and allow the scriptlets to be found\nnaturally by using symlinks to a single \"vimdiff\" scriptlet.  This\ncauses the --tool-help option to correctly list vimdiff, vimdiff2,\ngvimdiff, and gvimdiff2 in its output.\n\nSigned-off-by: David Aguilar <davvid@gmail.com>\n---\n git-mergetool--lib.sh       | 9 +--------\n mergetools/gvimdiff         | 1 +\n mergetools/gvimdiff2        | 1 +\n mergetools/{vim => vimdiff} | 0\n mergetools/vimdiff2         | 1 +\n 5 files changed, 4 insertions(+), 8 deletions(-)\n create mode 120000 mergetools/gvimdiff\n create mode 120000 mergetools/gvimdiff2\n rename mergetools/{vim => vimdiff} (100%)\n create mode 120000 mergetools/vimdiff2\n\ndiff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh\nindex 4c1e129..db8218a 100644\n--- a/git-mergetool--lib.sh\n+++ b/git-mergetool--lib.sh\n@@ -44,14 +44,7 @@ valid_tool () {\n }\n \n setup_tool () {\n-\tcase \"$1\" in\n-\tvim*|gvim*)\n-\t\ttool=vim\n-\t\t;;\n-\t*)\n-\t\ttool=\"$1\"\n-\t\t;;\n-\tesac\n+\ttool=\"$1\"\n \tmergetools=\"$(git --exec-path)/mergetools\"\n \n \t# Load the default definitions\ndiff --git a/mergetools/gvimdiff b/mergetools/gvimdiff\nnew file mode 120000\nindex 0000000..2a72558\n--- /dev/null\n+++ b/mergetools/gvimdiff\n@@ -0,0 +1 @@\n+vimdiff\n\\ No newline at end of file\ndiff --git a/mergetools/gvimdiff2 b/mergetools/gvimdiff2\nnew file mode 120000\nindex 0000000..2a72558\n--- /dev/null\n+++ b/mergetools/gvimdiff2\n@@ -0,0 +1 @@\n+vimdiff\n\\ No newline at end of file\ndiff --git a/mergetools/vim b/mergetools/vimdiff\nsimilarity index 100%\nrename from mergetools/vim\nrename to mergetools/vimdiff\ndiff --git a/mergetools/vimdiff2 b/mergetools/vimdiff2\nnew file mode 120000\nindex 0000000..2a72558\n--- /dev/null\n+++ b/mergetools/vimdiff2\n@@ -0,0 +1 @@\n+vimdiff\n\\ No newline at end of file\n-- \n1.8.1.1.367.g22b1720.dirty\n"},{"id":"207784","messageId":"1359107034-14606-8-git-send-email-davvid@gmail.com","threadId":"32723","inReplyTo":"1359107034-14606-1-git-send-email-davvid@gmail.com","subject":"[PATCH 7/7] mergetool--lib: Improve show_tool_help() output","fromName":"David Aguilar","fromEmail":"davvid@gmail.com","sentAt":"2013-01-25T09:43:54Z","receivedAt":"2013-01-25T09:43:54Z","isPatch":true,"sender":{"key":"davvid@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13196?v=4"},"body":"Check the can_diff and can_merge functions before deciding whether to\nadd the tool to the available/unavailable lists.  This makes --tool-help context-\nsensitive so that \"git mergetool --tool-help\" displays merge tools only\nand \"git difftool --tool-help\" displays diff tools only.\n\nSigned-off-by: David Aguilar <davvid@gmail.com>\n---\n git-mergetool--lib.sh | 26 +++++++++++++++++++++-----\n 1 file changed, 21 insertions(+), 5 deletions(-)\n\ndiff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh\nindex db8218a..c547c59 100644\n--- a/git-mergetool--lib.sh\n+++ b/git-mergetool--lib.sh\n@@ -168,17 +168,33 @@ list_merge_tool_candidates () {\n }\n \n show_tool_help () {\n-\tlist_merge_tool_candidates\n \tunavailable= available= LF='\n '\n-\tfor i in $tools\n+\n+\tscriptlets=\"$(git --exec-path)\"/mergetools\n+\tfor i in \"$scriptlets\"/*\n \tdo\n-\t\tmerge_tool_path=$(translate_merge_tool_path \"$i\")\n+\t\t. \"$scriptlets\"/defaults\n+\t\t. \"$i\"\n+\n+\t\ttool=\"$(basename \"$i\")\"\n+\t\tif test \"$tool\" = \"defaults\"\n+\t\tthen\n+\t\t\tcontinue\n+\t\telif merge_mode && ! can_merge\n+\t\tthen\n+\t\t\tcontinue\n+\t\telif diff_mode && ! can_diff\n+\t\tthen\n+\t\t\tcontinue\n+\t\tfi\n+\n+\t\tmerge_tool_path=$(translate_merge_tool_path \"$tool\")\n \t\tif type \"$merge_tool_path\" >/dev/null 2>&1\n \t\tthen\n-\t\t\tavailable=\"$available$i$LF\"\n+\t\t\tavailable=\"$available$tool$LF\"\n \t\telse\n-\t\t\tunavailable=\"$unavailable$i$LF\"\n+\t\t\tunavailable=\"$unavailable$tool$LF\"\n \t\tfi\n \tdone\n \n-- \n1.8.1.1.367.g22b1720.dirty\n"},{"id":"207794","messageId":"51025D1C.2030307@gmail.com","threadId":"32723","inReplyTo":"1359107034-14606-7-git-send-email-davvid@gmail.com","subject":"Re: [PATCH 6/7] mergetools: Fix difftool/mergetool --tool-help listing for vim","fromName":"Sebastian Schuberth","fromEmail":"sschuberth@gmail.com","sentAt":"2013-01-25T10:23:24Z","receivedAt":"2013-01-25T10:23:24Z","isPatch":true,"sender":{"key":"sschuberth@gmail.com","avatar":"https://avatars.githubusercontent.com/u/349154?v=4"},"body":"On 2013/01/25 10:43 , David Aguilar wrote:\n\n> Remove the exception for \"vim\" and allow the scriptlets to be found\n> naturally by using symlinks to a single \"vimdiff\" scriptlet.  This\n\nI guess that won't work on platforms where Git does not support \nsymlinks, then, like Windows. But Windows has (g)vimdiff, so loosing \nthese on that platform would not be so nice.\n\n-- \nSebastian Schuberth\n"},{"id":"207795","messageId":"CAJDDKr4yb1-C9W1ZnKxHUk7WKpq-EN_YiR6diHCm5DVtQWzeCQ@mail.gmail.com","threadId":"32723","inReplyTo":"51025D1C.2030307@gmail.com","subject":"Re: [PATCH 6/7] mergetools: Fix difftool/mergetool --tool-help listing for vim","fromName":"David Aguilar","fromEmail":"davvid@gmail.com","sentAt":"2013-01-25T10:28:22Z","receivedAt":"2013-01-25T10:28:22Z","isPatch":true,"sender":{"key":"davvid@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13196?v=4"},"body":"On Fri, Jan 25, 2013 at 2:23 AM, Sebastian Schuberth\n<sschuberth@gmail.com> wrote:\n> On 2013/01/25 10:43 , David Aguilar wrote:\n>\n>> Remove the exception for \"vim\" and allow the scriptlets to be found\n>> naturally by using symlinks to a single \"vimdiff\" scriptlet.  This\n>\n>\n> I guess that won't work on platforms where Git does not support symlinks,\n> then, like Windows. But Windows has (g)vimdiff, so loosing these on that\n> platform would not be so nice.\n\nI thought Git did something sensible there like create a normal file?\n\nIf not then I was thinking we could add a provides_tools() function that\neach scriptlet could supply.\n-- \nDavid\n"},{"id":"207796","messageId":"CAHGBnuMPQWr8Z9jeQmHs7wFN5kf=MwBEEDy_M-QeY0mRnSke_g@mail.gmail.com","threadId":"32723","inReplyTo":"CAJDDKr4yb1-C9W1ZnKxHUk7WKpq-EN_YiR6diHCm5DVtQWzeCQ@mail.gmail.com","subject":"Re: [PATCH 6/7] mergetools: Fix difftool/mergetool --tool-help listing for vim","fromName":"Sebastian Schuberth","fromEmail":"sschuberth@gmail.com","sentAt":"2013-01-25T10:34:35Z","receivedAt":"2013-01-25T10:34:35Z","isPatch":true,"sender":{"key":"sschuberth@gmail.com","avatar":"https://avatars.githubusercontent.com/u/349154?v=4"},"body":"On Fri, Jan 25, 2013 at 11:28 AM, David Aguilar <davvid@gmail.com> wrote:\n\n> I thought Git did something sensible there like create a normal file?\n\nIt does not. Also see my answer over here:\nhttp://stackoverflow.com/questions/11412028/git-not-storing-symlink-as-a-symlink-on-windows/11412341#11412341\n\n> If not then I was thinking we could add a provides_tools() function that\n> each scriptlet could supply.\n\nOr maybe simply introduce scriptlets that do nothing but include /\ninline another scriptlet?\n\n-- \nSebastian Schuberth\n"},{"id":"207798","messageId":"20130125103845.GX7498@serenity.lan","threadId":"32723","inReplyTo":"1359107034-14606-7-git-send-email-davvid@gmail.com","subject":"Re: [PATCH 6/7] mergetools: Fix difftool/mergetool --tool-help listing for vim","fromName":"John Keeping","fromEmail":"john@keeping.me.uk","sentAt":"2013-01-25T10:38:45Z","receivedAt":"2013-01-25T10:38:45Z","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:43:53AM -0800, David Aguilar wrote:\n> \"git difftool --tool-help\" and \"git mergetool --tool-help\" incorreclty\n> list \"vim\" as being an unavailable tool.  This is because they attempt\n> to find a tool named according to the mergetool scriptlet instead of the Git-\n> recognized tool name.\n\nActually, after my patches both git-difftool and git-mergetool get this\nright since list_merge_tool_candidates lists vimdiff and gvimdiff.\n\n> vimdiff, vimdiff2, gvimdiff, and gvimdiff2 are all provided by the \"vim\"\n> scriptlet.  This required git-mergetool--lib to special-case it when\n> setting up the tool.\n> \n> Remove the exception for \"vim\" and allow the scriptlets to be found\n> naturally by using symlinks to a single \"vimdiff\" scriptlet.\n\nI wonder if it would be better to make these single-line scripts instead\nof symlinks:\n\n    . \"$MERGE_TOOLS_DIR\"/vimdiff\n\nwhere we make git-mergetool--lib.sh export:\n\n    MERGE_TOOLS_DIR=$(git --exec-path)/mergetools\n\n\nJohn\n"},{"id":"207799","messageId":"CAJDDKr5ZsqO+PFoUabsZObgvG8jUBfTKL1HmVsn77ZhzsRZk-Q@mail.gmail.com","threadId":"32723","inReplyTo":"20130125103845.GX7498@serenity.lan","subject":"Re: [PATCH 6/7] mergetools: Fix difftool/mergetool --tool-help listing for vim","fromName":"David Aguilar","fromEmail":"davvid@gmail.com","sentAt":"2013-01-25T10:40:24Z","receivedAt":"2013-01-25T10:40:24Z","isPatch":true,"sender":{"key":"davvid@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13196?v=4"},"body":"On Fri, Jan 25, 2013 at 2:38 AM, John Keeping <john@keeping.me.uk> wrote:\n> On Fri, Jan 25, 2013 at 01:43:53AM -0800, David Aguilar wrote:\n>> \"git difftool --tool-help\" and \"git mergetool --tool-help\" incorreclty\n>> list \"vim\" as being an unavailable tool.  This is because they attempt\n>> to find a tool named according to the mergetool scriptlet instead of the Git-\n>> recognized tool name.\n>\n> Actually, after my patches both git-difftool and git-mergetool get this\n> right since list_merge_tool_candidates lists vimdiff and gvimdiff.\n>\n>> vimdiff, vimdiff2, gvimdiff, and gvimdiff2 are all provided by the \"vim\"\n>> scriptlet.  This required git-mergetool--lib to special-case it when\n>> setting up the tool.\n>>\n>> Remove the exception for \"vim\" and allow the scriptlets to be found\n>> naturally by using symlinks to a single \"vimdiff\" scriptlet.\n>\n> I wonder if it would be better to make these single-line scripts instead\n> of symlinks:\n>\n>     . \"$MERGE_TOOLS_DIR\"/vimdiff\n>\n> where we make git-mergetool--lib.sh export:\n>\n>     MERGE_TOOLS_DIR=$(git --exec-path)/mergetools\n\nThat sounds like the way to go.\n-- \nDavid\n"},{"id":"207803","messageId":"CAHGBnuNWo_+HY9Vwrze+zxDS9zh6BBR4q6Av0q=TaDibHpaPzA@mail.gmail.com","threadId":"32723","inReplyTo":"CAHGBnuMPQWr8Z9jeQmHs7wFN5kf=MwBEEDy_M-QeY0mRnSke_g@mail.gmail.com","subject":"Re: [PATCH 6/7] mergetools: Fix difftool/mergetool --tool-help listing for vim","fromName":"Sebastian Schuberth","fromEmail":"sschuberth@gmail.com","sentAt":"2013-01-25T11:39:04Z","receivedAt":"2013-01-25T11:39:04Z","isPatch":true,"sender":{"key":"sschuberth@gmail.com","avatar":"https://avatars.githubusercontent.com/u/349154?v=4"},"body":"On Fri, Jan 25, 2013 at 11:34 AM, Sebastian Schuberth\n<sschuberth@gmail.com> wrote:\n\n>> I thought Git did something sensible there like create a normal file?\n>\n> It does not. Also see my answer over here:\n> http://stackoverflow.com/questions/11412028/git-not-storing-symlink-as-a-symlink-on-windows/11412341#11412341\n\nThis one might be the even more appropriate answer:\nhttp://stackoverflow.com/questions/11662868/what-happens-when-i-clone-a-repository-with-symlinks-on-windows/11664406#11664406\n\n-- \nSebastian Schuberth\n"},{"id":"207826","messageId":"7vd2wt3wo4.fsf@alter.siamese.dyndns.org","threadId":"32723","inReplyTo":"CAJDDKr5ZsqO+PFoUabsZObgvG8jUBfTKL1HmVsn77ZhzsRZk-Q@mail.gmail.com","subject":"Re: [PATCH 6/7] mergetools: Fix difftool/mergetool --tool-help listing for vim","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-01-25T19:11:23Z","receivedAt":"2013-01-25T19:11:23Z","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> On Fri, Jan 25, 2013 at 2:38 AM, John Keeping <john@keeping.me.uk> wrote:\n>> On Fri, Jan 25, 2013 at 01:43:53AM -0800, David Aguilar wrote:\n>>> \"git difftool --tool-help\" and \"git mergetool --tool-help\" incorreclty\n>>> list \"vim\" as being an unavailable tool.  This is because they attempt\n>>> to find a tool named according to the mergetool scriptlet instead of the Git-\n>>> recognized tool name.\n>>\n>> Actually, after my patches both git-difftool and git-mergetool get this\n>> right since list_merge_tool_candidates lists vimdiff and gvimdiff.\n>>\n>>> vimdiff, vimdiff2, gvimdiff, and gvimdiff2 are all provided by the \"vim\"\n>>> scriptlet.  This required git-mergetool--lib to special-case it when\n>>> setting up the tool.\n>>>\n>>> Remove the exception for \"vim\" and allow the scriptlets to be found\n>>> naturally by using symlinks to a single \"vimdiff\" scriptlet.\n>>\n>> I wonder if it would be better to make these single-line scripts instead\n>> of symlinks:\n>>\n>>     . \"$MERGE_TOOLS_DIR\"/vimdiff\n>>\n>> where we make git-mergetool--lib.sh export:\n>>\n>>     MERGE_TOOLS_DIR=$(git --exec-path)/mergetools\n>\n> That sounds like the way to go.\n\nYup, I'll expect a reroll of this one and possibly the next one (I\nhaven't read yet).\n\n1-5/7 looked all very sensible.\n\nThanks.\n"},{"id":"207829","messageId":"20130125195446.GA7498@serenity.lan","threadId":"32723","inReplyTo":"1359107034-14606-8-git-send-email-davvid@gmail.com","subject":"Re: [PATCH 7/7] mergetool--lib: Improve show_tool_help() output","fromName":"John Keeping","fromEmail":"john@keeping.me.uk","sentAt":"2013-01-25T19:54:46Z","receivedAt":"2013-01-25T19:54:46Z","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:43:54AM -0800, David Aguilar wrote:\n> Check the can_diff and can_merge functions before deciding whether to\n> add the tool to the available/unavailable lists.  This makes --tool-help context-\n> sensitive so that \"git mergetool --tool-help\" displays merge tools only\n> and \"git difftool --tool-help\" displays diff tools only.\n\nThis log message is misleading - the existing code in\nlist_merge_tool_candidates already filters the tools like this, so the\nchange is more:\n\n    mergetool--lib: don't use a hardcoded list for \"--tool-help\"\n\n    Instead of using a list of tools in list_merge_tool_candidates, list\n    the available scriptlets and query each of those to know whether it\n    applies to diff mode and/or merge mode.\n\n    guess_merge_tool still relies on list_merge_tool_candidates so we\n    can't remove that function now.\n\n\nThe patch seems to do the right thing, although I have a couple of minor\nnits...\n\n> Signed-off-by: David Aguilar <davvid@gmail.com>\n> ---\n>  git-mergetool--lib.sh | 26 +++++++++++++++++++++-----\n>  1 file changed, 21 insertions(+), 5 deletions(-)\n> \n> diff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh\n> index db8218a..c547c59 100644\n> --- a/git-mergetool--lib.sh\n> +++ b/git-mergetool--lib.sh\n> @@ -168,17 +168,33 @@ list_merge_tool_candidates () {\n>  }\n>  \n>  show_tool_help () {\n> -\tlist_merge_tool_candidates\n>  \tunavailable= available= LF='\n>  '\n> -\tfor i in $tools\n> +\n> +\tscriptlets=\"$(git --exec-path)\"/mergetools\n> +\tfor i in \"$scriptlets\"/*\n>  \tdo\n> -\t\tmerge_tool_path=$(translate_merge_tool_path \"$i\")\n> +\t\t. \"$scriptlets\"/defaults\n> +\t\t. \"$i\"\n> +\n> +\t\ttool=\"$(basename \"$i\")\"\n\nQuotes are unnecessary here.\n\n> +\t\tif test \"$tool\" = \"defaults\"\n> +\t\tthen\n> +\t\t\tcontinue\n> +\t\telif merge_mode && ! can_merge\n> +\t\tthen\n> +\t\t\tcontinue\n> +\t\telif diff_mode && ! can_diff\n> +\t\tthen\n> +\t\t\tcontinue\n> +\t\tfi\n\nWould this be better as:\n\n    test \"$tool\" = \"defaults\" && continue\n\n    can_merge || ! merge_mode || continue\n    can_diff || ! diff_mode || continue\n\nor is that a bit too concise?\n\nI'd prefer to see two separate if statements either way since the \"test\n$tool = defaults\" case is different from the \"does it apply to the\ncurrent mode?\" case.  The \"$tool = defaults\" case could even move to the\ntop of the loop.\n\n> +\t\tmerge_tool_path=$(translate_merge_tool_path \"$tool\")\n>  \t\tif type \"$merge_tool_path\" >/dev/null 2>&1\n>  \t\tthen\n> -\t\t\tavailable=\"$available$i$LF\"\n> +\t\t\tavailable=\"$available$tool$LF\"\n>  \t\telse\n> -\t\t\tunavailable=\"$unavailable$i$LF\"\n> +\t\t\tunavailable=\"$unavailable$tool$LF\"\n>  \t\tfi\n>  \tdone\n"},{"id":"207830","messageId":"7vwqv12fjk.fsf@alter.siamese.dyndns.org","threadId":"32723","inReplyTo":"20130125195446.GA7498@serenity.lan","subject":"Re: [PATCH 7/7] mergetool--lib: Improve show_tool_help() output","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-01-25T20:06:39Z","receivedAt":"2013-01-25T20:06:39Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"John Keeping <john@keeping.me.uk> writes:\n\n>> +\t\ttool=\"$(basename \"$i\")\"\n>\n> Quotes are unnecessary here.\n\nYeah, the outer quotes aren't needed; the inner ones are.\n\n>> +\t\tif test \"$tool\" = \"defaults\"\n>> +\t\tthen\n>> +\t\t\tcontinue\n>> +\t\telif merge_mode && ! can_merge\n>> +\t\tthen\n>> +\t\t\tcontinue\n>> +\t\telif diff_mode && ! can_diff\n>> +\t\tthen\n>> +\t\t\tcontinue\n>> +\t\tfi\n>\n> Would this be better as:\n>\n>     test \"$tool\" = \"defaults\" && continue\n>\n>     can_merge || ! merge_mode || continue\n>     can_diff || ! diff_mode || continue\n>\n> or is that a bit too concise?\n\nIt is beyond \"too concise\"; it is unreadable, and more importantly,\nthe latter two lines are illogical (why do you even ask if it can be\nused for merging, before asking merge_mode to see if the answer to\nthat question matters to you?)\n"},{"id":"207831","messageId":"20130125200807.GB7498@serenity.lan","threadId":"32723","inReplyTo":"20130125195446.GA7498@serenity.lan","subject":"Re: [PATCH 7/7] mergetool--lib: Improve show_tool_help() output","fromName":"John Keeping","fromEmail":"john@keeping.me.uk","sentAt":"2013-01-25T20:08:07Z","receivedAt":"2013-01-25T20:08:07Z","isPatch":true,"sender":{"key":"john@keeping.me.uk","avatar":"https://avatars.githubusercontent.com/u/1702081?v=4"},"body":"On Fri, Jan 25, 2013 at 07:54:46PM +0000, John Keeping wrote:\n> On Fri, Jan 25, 2013 at 01:43:54AM -0800, David Aguilar wrote:\n> > Check the can_diff and can_merge functions before deciding whether to\n> > add the tool to the available/unavailable lists.  This makes --tool-help context-\n> > sensitive so that \"git mergetool --tool-help\" displays merge tools only\n> > and \"git difftool --tool-help\" displays diff tools only.\n> \n> This log message is misleading - the existing code in\n> list_merge_tool_candidates already filters the tools like this, so the\n> change is more:\n> \n>     mergetool--lib: don't use a hardcoded list for \"--tool-help\"\n> \n>     Instead of using a list of tools in list_merge_tool_candidates, list\n>     the available scriptlets and query each of those to know whether it\n>     applies to diff mode and/or merge mode.\n> \n>     guess_merge_tool still relies on list_merge_tool_candidates so we\n>     can't remove that function now.\n> \n> \n> The patch seems to do the right thing, although I have a couple of minor\n> nits...\n> \n> > Signed-off-by: David Aguilar <davvid@gmail.com>\n> > ---\n> >  git-mergetool--lib.sh | 26 +++++++++++++++++++++-----\n> >  1 file changed, 21 insertions(+), 5 deletions(-)\n> > \n> > diff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh\n> > index db8218a..c547c59 100644\n> > --- a/git-mergetool--lib.sh\n> > +++ b/git-mergetool--lib.sh\n> > @@ -168,17 +168,33 @@ list_merge_tool_candidates () {\n> >  }\n> >  \n> >  show_tool_help () {\n> > -\tlist_merge_tool_candidates\n> >  \tunavailable= available= LF='\n> >  '\n> > -\tfor i in $tools\n> > +\n> > +\tscriptlets=\"$(git --exec-path)\"/mergetools\n> > +\tfor i in \"$scriptlets\"/*\n> >  \tdo\n> > -\t\tmerge_tool_path=$(translate_merge_tool_path \"$i\")\n> > +\t\t. \"$scriptlets\"/defaults\n> > +\t\t. \"$i\"\n> > +\n> > +\t\ttool=\"$(basename \"$i\")\"\n> \n> Quotes are unnecessary here.\n> \n> > +\t\tif test \"$tool\" = \"defaults\"\n> > +\t\tthen\n> > +\t\t\tcontinue\n> > +\t\telif merge_mode && ! can_merge\n> > +\t\tthen\n> > +\t\t\tcontinue\n> > +\t\telif diff_mode && ! can_diff\n> > +\t\tthen\n> > +\t\t\tcontinue\n> > +\t\tfi\n> \n> Would this be better as:\n> \n>     test \"$tool\" = \"defaults\" && continue\n> \n>     can_merge || ! merge_mode || continue\n>     can_diff || ! diff_mode || continue\n> \n> or is that a bit too concise?\n> \n> I'd prefer to see two separate if statements either way since the \"test\n> $tool = defaults\" case is different from the \"does it apply to the\n> current mode?\" case.  The \"$tool = defaults\" case could even move to the\n> top of the loop.\n> \n> > +\t\tmerge_tool_path=$(translate_merge_tool_path \"$tool\")\n\nActually, can we just change all of the above part of the loop to:\n\n\ttest \"$tool\" = defaults && continue\n\n\tmerge_tool_path=$(\n\t\tsetup_tool \"$tool\" >/dev/null 2>&1 &&\n\t\ttranslate_merge_tool_path \"$tool\"\n\t) || continue\n\n> >  \t\tif type \"$merge_tool_path\" >/dev/null 2>&1\n> >  \t\tthen\n> > -\t\t\tavailable=\"$available$i$LF\"\n> > +\t\t\tavailable=\"$available$tool$LF\"\n> >  \t\telse\n> > -\t\t\tunavailable=\"$unavailable$i$LF\"\n> > +\t\t\tunavailable=\"$unavailable$tool$LF\"\n> >  \t\tfi\n> >  \tdone\n> --\n> To unsubscribe from this list: send the line \"unsubscribe git\" in\n> the body of a message to majordomo@vger.kernel.org\n> More majordomo info at  http://vger.kernel.org/majordomo-info.html\n"},{"id":"207832","messageId":"7vpq0t2f2t.fsf@alter.siamese.dyndns.org","threadId":"32723","inReplyTo":"20130125200807.GB7498@serenity.lan","subject":"Re: [PATCH 7/7] mergetool--lib: Improve show_tool_help() output","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-01-25T20:16:42Z","receivedAt":"2013-01-25T20:16:42Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"John Keeping <john@keeping.me.uk> writes:\n\n> Actually, can we just change all of the above part of the loop to:\n>\n> \ttest \"$tool\" = defaults && continue\n>\n> \tmerge_tool_path=$(\n> \t\tsetup_tool \"$tool\" >/dev/null 2>&1 &&\n> \t\ttranslate_merge_tool_path \"$tool\"\n> \t) || continue\n\nMeaning \"setup_tool ought to know which mode we are in and should\nfail if we are in merge mode and it does not support merging\"?  That\nline of reasoning makes tons of sense to me, compared to this script\nimplementing that logic for these scriptlets.\n\nHow/when does translate_merge_tool_path fail?\n\n>\n>> >  \t\tif type \"$merge_tool_path\" >/dev/null 2>&1\n>> >  \t\tthen\n>> > -\t\t\tavailable=\"$available$i$LF\"\n>> > +\t\t\tavailable=\"$available$tool$LF\"\n>> >  \t\telse\n>> > -\t\t\tunavailable=\"$unavailable$i$LF\"\n>> > +\t\t\tunavailable=\"$unavailable$tool$LF\"\n>> >  \t\tfi\n>> >  \tdone\n>> --\n>> To unsubscribe from this list: send the line \"unsubscribe git\" in\n>> the body of a message to majordomo@vger.kernel.org\n>> More majordomo info at  http://vger.kernel.org/majordomo-info.html\n"},{"id":"207837","messageId":"20130125204619.GC7498@serenity.lan","threadId":"32723","inReplyTo":"7vpq0t2f2t.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 7/7] mergetool--lib: Improve show_tool_help() output","fromName":"John Keeping","fromEmail":"john@keeping.me.uk","sentAt":"2013-01-25T20:46:19Z","receivedAt":"2013-01-25T20:46:19Z","isPatch":true,"sender":{"key":"john@keeping.me.uk","avatar":"https://avatars.githubusercontent.com/u/1702081?v=4"},"body":"On Fri, Jan 25, 2013 at 12:16:42PM -0800, Junio C Hamano wrote:\n> John Keeping <john@keeping.me.uk> writes:\n> \n> > Actually, can we just change all of the above part of the loop to:\n> >\n> > \ttest \"$tool\" = defaults && continue\n> >\n> > \tmerge_tool_path=$(\n> > \t\tsetup_tool \"$tool\" >/dev/null 2>&1 &&\n> > \t\ttranslate_merge_tool_path \"$tool\"\n> > \t) || continue\n> \n> Meaning \"setup_tool ought to know which mode we are in and should\n> fail if we are in merge mode and it does not support merging\"?  That\n> line of reasoning makes tons of sense to me, compared to this script\n> implementing that logic for these scriptlets.\n\nYes, that's part of what setup_tool does.  It actually calls \"exit\" if\nthe \"mode? && can_mode\" test fails, which is why we need to call it in\nthe subshell.\n\nI think this would get even better if we add a preparatory patch like\nthis, so we can just call setup_tool and then set merge_tool_path:\n\n-- >8 --\n\ndiff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh\nindex 888ae3e..4644cbf 100644\n--- a/git-mergetool--lib.sh\n+++ b/git-mergetool--lib.sh\n@@ -67,11 +67,11 @@ setup_tool () {\n \tif merge_mode && ! can_merge\n \tthen\n \t\techo \"error: '$tool' can not be used to resolve merges\" >&2\n-\t\texit 1\n+\t\treturn 1\n \telif diff_mode && ! can_diff\n \tthen\n \t\techo \"error: '$tool' can only be used to resolve merges\" >&2\n-\t\texit 1\n+\t\treturn 1\n \tfi\n \treturn 0\n }\n@@ -100,7 +100,7 @@ run_merge_tool () {\n \tstatus=0\n \n \t# Bring tool-specific functions into scope\n-\tsetup_tool \"$1\"\n+\tsetup_tool \"$1\" || return\n \n \tif merge_mode\n \tthen\n\n-- 8< --\n \n> How/when does translate_merge_tool_path fail?\n\nIt doesn't - the \"|| continue\" is to catch errors from setup_tool.\n\n\nJohn\n"},{"id":"207838","messageId":"7vlibh2d8a.fsf@alter.siamese.dyndns.org","threadId":"32723","inReplyTo":"20130125204619.GC7498@serenity.lan","subject":"Re: [PATCH 7/7] mergetool--lib: Improve show_tool_help() output","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-01-25T20:56:37Z","receivedAt":"2013-01-25T20:56:37Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"John Keeping <john@keeping.me.uk> writes:\n\n> On Fri, Jan 25, 2013 at 12:16:42PM -0800, Junio C Hamano wrote:\n>> John Keeping <john@keeping.me.uk> writes:\n>> \n>> > Actually, can we just change all of the above part of the loop to:\n>> >\n>> > \ttest \"$tool\" = defaults && continue\n>> >\n>> > \tmerge_tool_path=$(\n>> > \t\tsetup_tool \"$tool\" >/dev/null 2>&1 &&\n>> > \t\ttranslate_merge_tool_path \"$tool\"\n>> > \t) || continue\n>> \n>> Meaning \"setup_tool ought to know which mode we are in and should\n>> fail if we are in merge mode and it does not support merging\"?  That\n>> line of reasoning makes tons of sense to me, compared to this script\n>> implementing that logic for these scriptlets.\n>\n> Yes, that's part of what setup_tool does.  It actually calls \"exit\" if\n> the \"mode? && can_mode\" test fails, which is why we need to call it in\n> the subshell.\n>\n> I think this would get even better if we add a preparatory patch like\n> this, so we can just call setup_tool and then set merge_tool_path:\n>\n> -- >8 --\n>\n> diff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh\n> index 888ae3e..4644cbf 100644\n> --- a/git-mergetool--lib.sh\n> +++ b/git-mergetool--lib.sh\n> @@ -67,11 +67,11 @@ setup_tool () {\n>  \tif merge_mode && ! can_merge\n>  \tthen\n>  \t\techo \"error: '$tool' can not be used to resolve merges\" >&2\n> -\t\texit 1\n> +\t\treturn 1\n>  \telif diff_mode && ! can_diff\n>  \tthen\n>  \t\techo \"error: '$tool' can only be used to resolve merges\" >&2\n> -\t\texit 1\n> +\t\treturn 1\n>  \tfi\n>  \treturn 0\n>  }\n> @@ -100,7 +100,7 @@ run_merge_tool () {\n>  \tstatus=0\n>  \n>  \t# Bring tool-specific functions into scope\n> -\tsetup_tool \"$1\"\n> +\tsetup_tool \"$1\" || return\n>  \n>  \tif merge_mode\n>  \tthen\n>\n> -- 8< --\n>  \n>> How/when does translate_merge_tool_path fail?\n>\n> It doesn't - the \"|| continue\" is to catch errors from setup_tool.\n\nUgh.\n"},{"id":"207840","messageId":"20130125211601.GD7498@serenity.lan","threadId":"32723","inReplyTo":"7vlibh2d8a.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 7/7] mergetool--lib: Improve show_tool_help() output","fromName":"John Keeping","fromEmail":"john@keeping.me.uk","sentAt":"2013-01-25T21:16:01Z","receivedAt":"2013-01-25T21:16:01Z","isPatch":true,"sender":{"key":"john@keeping.me.uk","avatar":"https://avatars.githubusercontent.com/u/1702081?v=4"},"body":"On Fri, Jan 25, 2013 at 12:56:37PM -0800, Junio C Hamano wrote:\n> John Keeping <john@keeping.me.uk> writes:\n> \n> > On Fri, Jan 25, 2013 at 12:16:42PM -0800, Junio C Hamano wrote:\n> >> John Keeping <john@keeping.me.uk> writes:\n> >> \n> >> > Actually, can we just change all of the above part of the loop to:\n> >> >\n> >> > \ttest \"$tool\" = defaults && continue\n> >> >\n> >> > \tmerge_tool_path=$(\n> >> > \t\tsetup_tool \"$tool\" >/dev/null 2>&1 &&\n> >> > \t\ttranslate_merge_tool_path \"$tool\"\n> >> > \t) || continue\n> >> \n> >> Meaning \"setup_tool ought to know which mode we are in and should\n> >> fail if we are in merge mode and it does not support merging\"?  That\n> >> line of reasoning makes tons of sense to me, compared to this script\n> >> implementing that logic for these scriptlets.\n> >\n> > Yes, that's part of what setup_tool does.  It actually calls \"exit\" if\n> > the \"mode? && can_mode\" test fails, which is why we need to call it in\n> > the subshell.\n> >\n> > I think this would get even better if we add a preparatory patch like\n> > this, so we can just call setup_tool and then set merge_tool_path:\n> >\n> > -- >8 --\n> >\n> > diff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh\n> > index 888ae3e..4644cbf 100644\n> > --- a/git-mergetool--lib.sh\n> > +++ b/git-mergetool--lib.sh\n> > @@ -67,11 +67,11 @@ setup_tool () {\n> >  \tif merge_mode && ! can_merge\n> >  \tthen\n> >  \t\techo \"error: '$tool' can not be used to resolve merges\" >&2\n> > -\t\texit 1\n> > +\t\treturn 1\n> >  \telif diff_mode && ! can_diff\n> >  \tthen\n> >  \t\techo \"error: '$tool' can only be used to resolve merges\" >&2\n> > -\t\texit 1\n> > +\t\treturn 1\n> >  \tfi\n> >  \treturn 0\n> >  }\n> > @@ -100,7 +100,7 @@ run_merge_tool () {\n> >  \tstatus=0\n> >  \n> >  \t# Bring tool-specific functions into scope\n> > -\tsetup_tool \"$1\"\n> > +\tsetup_tool \"$1\" || return\n> >  \n> >  \tif merge_mode\n> >  \tthen\n> >\n> > -- 8< --\n> >  \n> >> How/when does translate_merge_tool_path fail?\n> >\n> > It doesn't - the \"|| continue\" is to catch errors from setup_tool.\n> \n> Ugh.\n\nIs that targeted at my suggestion at the top of this email or calling\nexit in setup_tool?\n\nWith the patch above, the block of code at the top becomes:\n\n \ttest \"$tool\" = defaults && continue\n\n \tsetup_tool \"$tool\" 2>/dev/null || continue\n \tmerge_tool_path=$(translate_merge_tool_path \"$tool\")\n\nwhich IMHO is pretty readable.\n\n\nJohn\n"},{"id":"207841","messageId":"7vbocd2auo.fsf@alter.siamese.dyndns.org","threadId":"32723","inReplyTo":"20130125211601.GD7498@serenity.lan","subject":"Re: [PATCH 7/7] mergetool--lib: Improve show_tool_help() output","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-01-25T21:47:59Z","receivedAt":"2013-01-25T21:47:59Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"John Keeping <john@keeping.me.uk> writes:\n\n>> > It doesn't - the \"|| continue\" is to catch errors from setup_tool.\n>> \n>> Ugh.\n>\n> Is that targeted at my suggestion at the top of this email or calling\n> exit in setup_tool?\n\nAt the fact that you had to go a convoluted route because you cannot\njust run setup_tool in subshell and do translate_merge_tool_path\nafter that, because the latter needs to look at the shell variable\nthe former sets.\n\n> With the patch above, the block of code at the top becomes:\n>\n>  \ttest \"$tool\" = defaults && continue\n>\n>  \tsetup_tool \"$tool\" 2>/dev/null || continue\n>  \tmerge_tool_path=$(translate_merge_tool_path \"$tool\")\n>\n> which IMHO is pretty readable.\n\nOf course it is.  The current callers of setup_tool may need some\nadjustments, but that should be fairly trivial, I hope.\n"},{"id":"207842","messageId":"20130125220222.GE7498@serenity.lan","threadId":"32723","inReplyTo":"7vbocd2auo.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 7/7] mergetool--lib: Improve show_tool_help() output","fromName":"John Keeping","fromEmail":"john@keeping.me.uk","sentAt":"2013-01-25T22:02:22Z","receivedAt":"2013-01-25T22:02: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:47:59PM -0800, Junio C Hamano wrote:\n> John Keeping <john@keeping.me.uk> writes:\n> > With the patch above, the block of code at the top becomes:\n> >\n> >  \ttest \"$tool\" = defaults && continue\n> >\n> >  \tsetup_tool \"$tool\" 2>/dev/null || continue\n> >  \tmerge_tool_path=$(translate_merge_tool_path \"$tool\")\n> >\n> > which IMHO is pretty readable.\n> \n> Of course it is.  The current callers of setup_tool may need some\n> adjustments, but that should be fairly trivial, I hope.\n\nThere are only two and one of them already seems like it doesn't want\nthe command to cause the script to exit.\n\nDavid, can you incorporate the following two patches when you re-roll?\nYour original 7/7 with the change above will want to build on 8/7.\n\n\nJohn\n"},{"id":"207843","messageId":"20130125220359.GF7498@serenity.lan","threadId":"32723","inReplyTo":"20130125220222.GE7498@serenity.lan","subject":"[PATCH 8/7] mergetool--lib: don't call \"exit\" in setup_tool","fromName":"John Keeping","fromEmail":"john@keeping.me.uk","sentAt":"2013-01-25T22:03:59Z","receivedAt":"2013-01-25T22:03:59Z","isPatch":true,"sender":{"key":"john@keeping.me.uk","avatar":"https://avatars.githubusercontent.com/u/1702081?v=4"},"body":"This will make it easier to use setup_tool in places where we expect\nthat the selected tool will not support the current mode.\n\nSigned-off-by: John Keeping <john@keeping.me.uk>\n---\n git-mergetool--lib.sh | 6 +++---\n 1 file changed, 3 insertions(+), 3 deletions(-)\n\ndiff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh\nindex 4c1e129..c6bd8ba 100644\n--- a/git-mergetool--lib.sh\n+++ b/git-mergetool--lib.sh\n@@ -67,11 +67,11 @@ setup_tool () {\n \tif merge_mode && ! can_merge\n \tthen\n \t\techo \"error: '$tool' can not be used to resolve merges\" >&2\n-\t\texit 1\n+\t\treturn 1\n \telif diff_mode && ! can_diff\n \tthen\n \t\techo \"error: '$tool' can only be used to resolve merges\" >&2\n-\t\texit 1\n+\t\treturn 1\n \tfi\n \treturn 0\n }\n@@ -100,7 +100,7 @@ run_merge_tool () {\n \tstatus=0\n \n \t# Bring tool-specific functions into scope\n-\tsetup_tool \"$1\"\n+\tsetup_tool \"$1\" || return\n \n \tif merge_mode\n \tthen\n-- \n1.8.1\n"},{"id":"207844","messageId":"20130125220442.GG7498@serenity.lan","threadId":"32723","inReplyTo":"20130125220222.GE7498@serenity.lan","subject":"[PATCH 9/7] mergetool--lib: fix path lookup in guess_merge_tool","fromName":"John Keeping","fromEmail":"john@keeping.me.uk","sentAt":"2013-01-25T22:04:42Z","receivedAt":"2013-01-25T22:04:42Z","isPatch":true,"sender":{"key":"john@keeping.me.uk","avatar":"https://avatars.githubusercontent.com/u/1702081?v=4"},"body":"guess_merge_tool calls translate_merge_tool_path in order to get the\ncorrect name of the tool to check whether it can be found on the user's\nsystem.  But this function is designed to be overridden by tool\nscriptlets so it does nothing if the relevant scriptlet has not been\nsourced.\n\nFix this by calling setup_tool before doing anything.\n\nSigned-off-by: John Keeping <john@keeping.me.uk>\n---\n git-mergetool--lib.sh | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh\nindex c6bd8ba..46860c5 100644\n--- a/git-mergetool--lib.sh\n+++ b/git-mergetool--lib.sh\n@@ -219,6 +219,7 @@ guess_merge_tool () {\n \t# Loop over each candidate and stop when a valid merge tool is found.\n \tfor i in $tools\n \tdo\n+\t\tsetup_tool \"$i\" 2>&1 || continue\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-- \n1.8.1\n"},{"id":"207855","messageId":"7vip6k23mk.fsf@alter.siamese.dyndns.org","threadId":"32723","inReplyTo":"20130125220359.GF7498@serenity.lan","subject":"Re: [PATCH 8/7] mergetool--lib: don't call \"exit\" in setup_tool","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-01-26T00:24:03Z","receivedAt":"2013-01-26T00:24:03Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Applying this one on top of 1/7 thru 5/7 and 7/7 seems to break\nt7610 rather badly.\n\n--- >8 ------ >8 ------ >8 ------ >8 ------ >8 ------ >8 ---\n...\nok 1 - setup\n\nexpecting success:\n    git checkout -b test1 branch1 &&\n    git submodule update -N &&\n    test_must_fail git merge master >/dev/null 2>&1 &&\n    ( yes \"\" | git mergetool both >/dev/null 2>&1 ) &&\n    ( yes \"\" | git mergetool file1 file1 ) &&\n    ( yes \"\" | git mergetool file2 \"spaced name\" >/dev/null 2>&1 ) &&\n    ( yes \"\" | git mergetool subdir/file3 >/dev/null 2>&1 ) &&\n    ( yes \"d\" | git mergetool file11 >/dev/null 2>&1 ) &&\n    ( yes \"d\" | git mergetool file12 >/dev/null 2>&1 ) &&\n    ( yes \"l\" | git mergetool submod >/dev/null 2>&1 ) &&\n    test \"$(cat file1)\" = \"master updated\" &&\n    test \"$(cat file2)\" = \"master new\" &&\n    test \"$(cat subdir/file3)\" = \"master new sub\" &&\n    test \"$(cat submod/bar)\" = \"branch1 submodule\" &&\n    git commit -m \"branch1 resolved with mergetool\"\n\nM       submod\nSwitched to a new branch 'test1'\nSubmodule path 'submod': checked out '39c7f044ed2e6a9cebd5266529badd181c8762b5'\nnot ok - 2 custom mergetool\n#\n#           git checkout -b test1 branch1 &&\n#           git submodule update -N &&\n#           test_must_fail git merge master >/dev/null 2>&1 &&\n#           ( yes \"\" | git mergetool both >/dev/null 2>&1 ) &&\n#           ( yes \"\" | git mergetool file1 file1 ) &&\n#           ( yes \"\" | git mergetool file2 \"spaced name\" >/dev/null 2>&1 ) &&\n#           ( yes \"\" | git mergetool subdir/file3 >/dev/null 2>&1 ) &&\n#           ( yes \"d\" | git mergetool file11 >/dev/null 2>&1 ) &&\n#           ( yes \"d\" | git mergetool file12 >/dev/null 2>&1 ) &&\n#           ( yes \"l\" | git mergetool submod >/dev/null 2>&1 ) &&\n#           test \"$(cat file1)\" = \"master updated\" &&\n#           test \"$(cat file2)\" = \"master new\" &&\n#           test \"$(cat subdir/file3)\" = \"master new sub\" &&\n#           test \"$(cat submod/bar)\" = \"branch1 submodule\" &&\n#           git commit -m \"branch1 resolved with mergetool\"\n#\n--- 8< ------ 8< ------ 8< ------ 8< ------ 8< ------ 8< ---\n\nDue to \">dev/null 2>&1\", all of the error clues are hidden, and I\ndidn't dig further to see which one was failing (this is why tests\nshouldn't do these in general).\n"},{"id":"207868","messageId":"CAJDDKr5aC6K7FqVB-EgmObUTJUfS3Qc5YOeG-0eZd289YCq1Og@mail.gmail.com","threadId":"32723","inReplyTo":"7vip6k23mk.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 8/7] mergetool--lib: don't call \"exit\" in setup_tool","fromName":"David Aguilar","fromEmail":"davvid@gmail.com","sentAt":"2013-01-26T07:01:08Z","receivedAt":"2013-01-26T07:01:08Z","isPatch":true,"sender":{"key":"davvid@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13196?v=4"},"body":"On Fri, Jan 25, 2013 at 4:24 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> Applying this one on top of 1/7 thru 5/7 and 7/7 seems to break\n> t7610 rather badly.\n\nI just sent a replacement for the vim/symlink issue stuff.\nI tried to keep the patch small.  John, can you rebase this\npatch on top of it?\n\n> --- >8 ------ >8 ------ >8 ------ >8 ------ >8 ------ >8 ---\n> ...\n> ok 1 - setup\n>\n> expecting success:\n>     git checkout -b test1 branch1 &&\n>     git submodule update -N &&\n>     test_must_fail git merge master >/dev/null 2>&1 &&\n>     ( yes \"\" | git mergetool both >/dev/null 2>&1 ) &&\n>     ( yes \"\" | git mergetool file1 file1 ) &&\n>     ( yes \"\" | git mergetool file2 \"spaced name\" >/dev/null 2>&1 ) &&\n>     ( yes \"\" | git mergetool subdir/file3 >/dev/null 2>&1 ) &&\n>     ( yes \"d\" | git mergetool file11 >/dev/null 2>&1 ) &&\n>     ( yes \"d\" | git mergetool file12 >/dev/null 2>&1 ) &&\n>     ( yes \"l\" | git mergetool submod >/dev/null 2>&1 ) &&\n>     test \"$(cat file1)\" = \"master updated\" &&\n>     test \"$(cat file2)\" = \"master new\" &&\n>     test \"$(cat subdir/file3)\" = \"master new sub\" &&\n>     test \"$(cat submod/bar)\" = \"branch1 submodule\" &&\n>     git commit -m \"branch1 resolved with mergetool\"\n>\n> M       submod\n> Switched to a new branch 'test1'\n> Submodule path 'submod': checked out '39c7f044ed2e6a9cebd5266529badd181c8762b5'\n> not ok - 2 custom mergetool\n> #\n> #           git checkout -b test1 branch1 &&\n> #           git submodule update -N &&\n> #           test_must_fail git merge master >/dev/null 2>&1 &&\n> #           ( yes \"\" | git mergetool both >/dev/null 2>&1 ) &&\n> #           ( yes \"\" | git mergetool file1 file1 ) &&\n> #           ( yes \"\" | git mergetool file2 \"spaced name\" >/dev/null 2>&1 ) &&\n> #           ( yes \"\" | git mergetool subdir/file3 >/dev/null 2>&1 ) &&\n> #           ( yes \"d\" | git mergetool file11 >/dev/null 2>&1 ) &&\n> #           ( yes \"d\" | git mergetool file12 >/dev/null 2>&1 ) &&\n> #           ( yes \"l\" | git mergetool submod >/dev/null 2>&1 ) &&\n> #           test \"$(cat file1)\" = \"master updated\" &&\n> #           test \"$(cat file2)\" = \"master new\" &&\n> #           test \"$(cat subdir/file3)\" = \"master new sub\" &&\n> #           test \"$(cat submod/bar)\" = \"branch1 submodule\" &&\n> #           git commit -m \"branch1 resolved with mergetool\"\n> #\n> --- 8< ------ 8< ------ 8< ------ 8< ------ 8< ------ 8< ---\n>\n> Due to \">dev/null 2>&1\", all of the error clues are hidden, and I\n> didn't dig further to see which one was failing (this is why tests\n> shouldn't do these in general).\n\n\n\n-- \nDavid\n"},{"id":"207875","messageId":"20130126121721.GI7498@serenity.lan","threadId":"32723","inReplyTo":"7vip6k23mk.fsf@alter.siamese.dyndns.org","subject":"[PATCH 1/2 v2] mergetool--lib: don't call \"exit\" in setup_tool","fromName":"John Keeping","fromEmail":"john@keeping.me.uk","sentAt":"2013-01-26T12:17:21Z","receivedAt":"2013-01-26T12:17:21Z","isPatch":true,"sender":{"key":"john@keeping.me.uk","avatar":"https://avatars.githubusercontent.com/u/1702081?v=4"},"body":"This will make it easier to use setup_tool in places where we expect\nthat the selected tool will not support the current mode.\n\nSigned-off-by: John Keeping <john@keeping.me.uk>\n---\nOn Fri, Jan 25, 2013 at 04:24:03PM -0800, Junio C Hamano wrote:\n> Applying this one on top of 1/7 thru 5/7 and 7/7 seems to break\n> t7610 rather badly.\n\nSorry about that.  The 'setup_tool' function should really be called\n'setup_builtin_tool' - it isn't necessary when a custom mergetool is\nconfigured and will return 1 when called with an argument that isn't a\nbuiltin tool from $GIT_EXEC_PATH/mergetools.\n\nThe change is the second hunk below which now wraps the call to\nsetup_tool in an if block as well as adding the \"|| return\".\n\n git-mergetool--lib.sh | 9 ++++++---\n 1 file changed, 6 insertions(+), 3 deletions(-)\n\ndiff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh\nindex 4c1e129..8a5eaff 100644\n--- a/git-mergetool--lib.sh\n+++ b/git-mergetool--lib.sh\n@@ -67,11 +67,11 @@ setup_tool () {\n \tif merge_mode && ! can_merge\n \tthen\n \t\techo \"error: '$tool' can not be used to resolve merges\" >&2\n-\t\texit 1\n+\t\treturn 1\n \telif diff_mode && ! can_diff\n \tthen\n \t\techo \"error: '$tool' can only be used to resolve merges\" >&2\n-\t\texit 1\n+\t\treturn 1\n \tfi\n \treturn 0\n }\n@@ -100,7 +100,10 @@ run_merge_tool () {\n \tstatus=0\n \n \t# Bring tool-specific functions into scope\n-\tsetup_tool \"$1\"\n+\tif test -z \"$merge_tool_path\"\n+\tthen\n+\t\tsetup_tool \"$1\" || return\n+\tfi\n \n \tif merge_mode\n \tthen\n-- \n1.8.1.1.367.ga9c3dd4.dirty\n"},{"id":"207879","messageId":"20130126135023.GJ7498@serenity.lan","threadId":"32723","inReplyTo":"20130126121721.GI7498@serenity.lan","subject":"[PATCH 1/2 v3] mergetool--lib: don't call \"exit\" in setup_tool","fromName":"John Keeping","fromEmail":"john@keeping.me.uk","sentAt":"2013-01-26T13:50:23Z","receivedAt":"2013-01-26T13:50:23Z","isPatch":true,"sender":{"key":"john@keeping.me.uk","avatar":"https://avatars.githubusercontent.com/u/1702081?v=4"},"body":"This will make it easier to use setup_tool in places where we expect\nthat the selected tool will not support the current mode.\n\nWe need to introduce a new return code for setup_tool to differentiate\nbetween the case of \"the selected tool is invalid\" and \"the selected\ntool is not a built-in\" since we must call setup_tool when a custom\n'merge.<tool>.path' is configured for a built-in tool but avoid failing\nwhen the configured tool is not a built-in.\n\nSigned-off-by: John Keeping <john@keeping.me.uk>\n---\n> On Fri, Jan 25, 2013 at 04:24:03PM -0800, Junio C Hamano wrote:\n> > Applying this one on top of 1/7 thru 5/7 and 7/7 seems to break\n> > t7610 rather badly.\n> \n> Sorry about that.  The 'setup_tool' function should really be called\n> 'setup_builtin_tool' - it isn't necessary when a custom mergetool is\n> configured and will return 1 when called with an argument that isn't a\n> builtin tool from $GIT_EXEC_PATH/mergetools.\n> \n> The change is the second hunk below which now wraps the call to\n> setup_tool in an if block as well as adding the \"|| return\".\n\nNow that I've run the entire test suite, that still wasn't correct since\nit did not correctly handle the case where the user overrides the path\nfor one of the built-in mergetools.\n\n git-mergetool--lib.sh | 23 ++++++++++++++++++++---\n 1 file changed, 20 insertions(+), 3 deletions(-)\n\ndiff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh\nindex 4c1e129..dd4f088 100644\n--- a/git-mergetool--lib.sh\n+++ b/git-mergetool--lib.sh\n@@ -58,7 +58,11 @@ setup_tool () {\n \t. \"$mergetools/defaults\"\n \tif ! test -f \"$mergetools/$tool\"\n \tthen\n-\t\treturn 1\n+\t\t# Use a special return code for this case since we want to\n+\t\t# source \"defaults\" even when an explicit tool path is\n+\t\t# configured since the user can use that to override the\n+\t\t# default path in the scriptlet.\n+\t\treturn 2\n \tfi\n \n \t# Load the redefined functions\n@@ -67,11 +71,11 @@ setup_tool () {\n \tif merge_mode && ! can_merge\n \tthen\n \t\techo \"error: '$tool' can not be used to resolve merges\" >&2\n-\t\texit 1\n+\t\treturn 1\n \telif diff_mode && ! can_diff\n \tthen\n \t\techo \"error: '$tool' can only be used to resolve merges\" >&2\n-\t\texit 1\n+\t\treturn 1\n \tfi\n \treturn 0\n }\n@@ -101,6 +105,19 @@ run_merge_tool () {\n \n \t# Bring tool-specific functions into scope\n \tsetup_tool \"$1\"\n+\texitcode=$?\n+\tcase $exitcode in\n+\t0)\n+\t\t:\n+\t\t;;\n+\t2)\n+\t\t# The configured tool is not a built-in tool.\n+\t\ttest -n \"$merge_tool_path\" || return 1\n+\t\t;;\n+\t*)\n+\t\treturn $exitcode\n+\t\t;;\n+\tesac\n \n \tif merge_mode\n \tthen\n-- \n1.8.1\n"}]}