{"thread":{"id":"50966","subject":"[PATCH 0/5] difftool and mergetool improvements","startedAt":"2019-04-22T05:07:43Z","lastAt":"2019-04-29T06:21:25Z","messageCount":48,"participants":["Denton Liu","Eric Sunshine","Jeff Hostetler","Junio C Hamano","David Aguilar"],"isPatch":true,"patchVersion":1,"patchTotal":5},"messages":[{"id":"374217","messageId":"cover.1555880168.git.liu.denton@gmail.com","threadId":"50966","inReplyTo":null,"subject":"[PATCH 0/5] difftool and mergetool improvements","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2019-04-22T05:07:38Z","receivedAt":"2019-04-22T05:07:43Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"I noticed earlier that when running 'git difftool --gui', we do not\nfallback to 'merge.guitool' when 'diff.guitool' isn't set, even though\nthis behaviour exists for when we invoke 'git difftool' (i.e. it falls\nback from diff.tool to merge.tool).\n\nWhile fixing this bug up, I noticed a few other places where we could do\nsome code cleanup/add tests so I did that as well.\n\nDenton Liu (5):\n t7610: add mergetool --gui tests\n mergetool: use get_merge_tool function\n mergetool: fallback to tool when guitool unavailable\n difftool: make --gui, --tool and --extcmd exclusive\n difftool: fallback on merge.guitool\n\n Documentation/git-difftool.txt | 4 ++-\n Documentation/git-mergetool--lib.txt | 5 +++-\n Documentation/git-mergetool.txt | 4 ++-\n builtin/difftool.c | 21 ++++++++------\n git-difftool--helper.sh | 2 +-\n git-mergetool--lib.sh | 33 ++++++++++++++--------\n git-mergetool.sh | 11 ++------\n t/t7610-mergetool.sh | 41 ++++++++++++++++++++++++++++\n t/t7800-difftool.sh | 24 ++++++++++++++++\n 9 files changed, 113 insertions(+), 32 deletions(-)\n\n-- \n2.21.0.967.gf85e14fd49\n\n"},{"id":"374218","messageId":"678f9b11fc7df7d3ee2050388574bcaea86af331.1555880168.git.liu.denton@gmail.com","threadId":"50966","inReplyTo":"cover.1555880168.git.liu.denton@gmail.com","subject":"[PATCH 1/5] t7610: add mergetool --gui tests","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2019-04-22T05:07:41Z","receivedAt":"2019-04-22T05:07:45Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"In 063f2bdbf7 (mergetool: accept -g/--[no-]gui as arguments,\n2018-10-24), mergetool was taught the --gui option but no tests were\nadded to ensure that it was working properly. Add a test to ensure that\nit works.\n\nSigned-off-by: Denton Liu \n---\n t/t7610-mergetool.sh | 22 ++++++++++++++++++++++\n 1 file changed, 22 insertions(+)\n\ndiff --git a/t/t7610-mergetool.sh b/t/t7610-mergetool.sh\nindex a9fb971615..5f37d7a1ff 100755\n--- a/t/t7610-mergetool.sh\n+++ b/t/t7610-mergetool.sh\n@@ -145,6 +145,28 @@ test_expect_success 'custom mergetool' '\n \tgit commit -m \"branch1 resolved with mergetool\"\n '\n \n+test_expect_success 'gui mergetool' '\n+\ttest_config merge.guitool myguitool &&\n+\ttest_config mergetool.myguitool.cmd \"(printf \\\"gui \\\" && cat \\\"\\$REMOTE\\\") >\\\"\\$MERGED\\\"\" &&\n+\ttest_config mergetool.myguitool.trustExitCode true &&\n+\ttest_when_finished \"git reset --hard\" &&\n+\tgit checkout -b test$test_count branch1 &&\n+\tgit submodule update -N &&\n+\ttest_must_fail git merge master >/dev/null 2>&1 &&\n+\t( yes \"\" | git mergetool --gui both >/dev/null 2>&1 ) &&\n+\t( yes \"\" | git mergetool -g file1 file1 ) &&\n+\t( yes \"\" | git mergetool --gui file2 \"spaced name\" >/dev/null 2>&1 ) &&\n+\t( yes \"\" | git mergetool --gui subdir/file3 >/dev/null 2>&1 ) &&\n+\t( yes \"d\" | git mergetool --gui file11 >/dev/null 2>&1 ) &&\n+\t( yes \"d\" | git mergetool --gui file12 >/dev/null 2>&1 ) &&\n+\t( yes \"l\" | git mergetool --gui submod >/dev/null 2>&1 ) &&\n+\ttest \"$(cat file1)\" = \"gui master updated\" &&\n+\ttest \"$(cat file2)\" = \"gui master new\" &&\n+\ttest \"$(cat subdir/file3)\" = \"gui master new sub\" &&\n+\ttest \"$(cat submod/bar)\" = \"branch1 submodule\" &&\n+\tgit commit -m \"branch1 resolved with mergetool\"\n+'\n+\n test_expect_success 'mergetool crlf' '\n \ttest_when_finished \"git reset --hard\" &&\n \t# This test_config line must go after the above reset line so that\n-- \n2.21.0.967.gf85e14fd49\n\n"},{"id":"374219","messageId":"692875cf4baaeee8b47fd7e95d0b787d1a08f64e.1555880168.git.liu.denton@gmail.com","threadId":"50966","inReplyTo":"cover.1555880168.git.liu.denton@gmail.com","subject":"[PATCH 2/5] mergetool: use get_merge_tool function","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2019-04-22T05:07:43Z","receivedAt":"2019-04-22T05:07:48Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"In git-mergetool, the logic for getting which merge tool to use is\nduplicated in git-mergetool--lib, except for the fact that it needs to\nknow whether the tool was guessed or not.\n\nRewrite `get_merge_tool` to return whether or not the tool was guessed\nand make git-mergetool call this function instead of duplicating the\nlogic. Also, let `$GIT_MERGETOOL_GUI` be set to determine whether or not\nthe guitool will be selected.\n\nSigned-off-by: Denton Liu \n---\n Documentation/git-mergetool--lib.txt | 5 ++++-\n git-difftool--helper.sh | 2 +-\n git-mergetool--lib.sh | 6 ++++--\n git-mergetool.sh | 11 +++--------\n 4 files changed, 12 insertions(+), 12 deletions(-)\n\ndiff --git a/Documentation/git-mergetool--lib.txt b/Documentation/git-mergetool--lib.txt\nindex 055550b2bc..c4f10209e0 100644\n--- a/Documentation/git-mergetool--lib.txt\n+++ b/Documentation/git-mergetool--lib.txt\n@@ -28,7 +28,10 @@ to define the operation mode for the functions listed below.\n FUNCTIONS\n ---------\n get_merge_tool::\n-\treturns a merge tool.\n+\treturns '$is_guessed:$merge_tool'. '$is_guessed' is 'true' if\n+\tthe tool was guessed, else 'false'. '$merge_tool' is the merge\n+\ttool to use. '$GIT_MERGETOOL_GUI' may be set to 'true' to search\n+\tfor the appropriate guitool.\n \n get_merge_tool_cmd::\n \treturns the custom command for a merge tool.\ndiff --git a/git-difftool--helper.sh b/git-difftool--helper.sh\nindex 7bfb6737df..78a0446668 100755\n--- a/git-difftool--helper.sh\n+++ b/git-difftool--helper.sh\n@@ -71,7 +71,7 @@ then\n \tthen\n \t\tmerge_tool=\"$GIT_DIFF_TOOL\"\n \telse\n-\t\tmerge_tool=\"$(get_merge_tool)\" || exit\n+\t\tmerge_tool=\"$(get_merge_tool | sed -e 's/^[a-z]*://')\" || exit\n \tfi\n fi\n \ndiff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh\nindex 83bf52494c..d5e2c6c5c6 100644\n--- a/git-mergetool--lib.sh\n+++ b/git-mergetool--lib.sh\n@@ -403,14 +403,16 @@ get_merge_tool_path () {\n }\n \n get_merge_tool () {\n+\tis_guessed=false\n \t# Check if a merge tool has been configured\n-\tmerge_tool=$(get_configured_merge_tool)\n+\tmerge_tool=$(get_configured_merge_tool $GIT_MERGETOOL_GUI)\n \t# Try to guess an appropriate merge tool if no tool has been set.\n \tif test -z \"$merge_tool\"\n \tthen\n \t\tmerge_tool=$(guess_merge_tool) || exit\n+\t\tis_guessed=true\n \tfi\n-\techo \"$merge_tool\"\n+\techo \"$is_guessed:$merge_tool\"\n }\n \n mergetool_find_win32_cmd () {\ndiff --git a/git-mergetool.sh b/git-mergetool.sh\nindex 01b9ad59b2..6ad8024e46 100755\n--- a/git-mergetool.sh\n+++ b/git-mergetool.sh\n@@ -449,14 +449,9 @@ main () {\n \n \tif test -z \"$merge_tool\"\n \tthen\n-\t\t# Check if a merge tool has been configured\n-\t\tmerge_tool=$(get_configured_merge_tool $gui_tool)\n-\t\t# Try to guess an appropriate merge tool if no tool has been set.\n-\t\tif test -z \"$merge_tool\"\n-\t\tthen\n-\t\t\tmerge_tool=$(guess_merge_tool) || exit\n-\t\t\tguessed_merge_tool=true\n-\t\tfi\n+\t\tIFS=':' read guessed_merge_tool merge_tool <<-EOF\n+\t\t$(GIT_MERGETOOL_GUI=$gui_tool get_merge_tool)\n+\t\tEOF\n \tfi\n \tmerge_keep_backup=\"$(git config --bool mergetool.keepBackup || echo true)\"\n \tmerge_keep_temporaries=\"$(git config --bool mergetool.keepTemporaries || echo false)\"\n-- \n2.21.0.967.gf85e14fd49\n\n"},{"id":"374220","messageId":"de1b897a11f96d2883907640c29313dd26886ea3.1555880168.git.liu.denton@gmail.com","threadId":"50966","inReplyTo":"cover.1555880168.git.liu.denton@gmail.com","subject":"[PATCH 3/5] mergetool: fallback to tool when guitool unavailable","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2019-04-22T05:07:45Z","receivedAt":"2019-04-22T05:07:51Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"In git-difftool, if the tool is called with --gui but `diff.guitool` is\nnot set, it falls back to `diff.tool`. Make git-mergetool also fallback\nfrom `merge.guitool` to `merge.tool` if the former is undefined.\n\nIf git-difftool were to use `get_configured_mergetool`, it would also\nget the fallback behaviour in the following precedence:\n\n1. diff.guitool\n2. merge.guitool\n3. diff.tool\n4. merge.tool\n\nSigned-off-by: Denton Liu \n---\n Documentation/git-mergetool.txt | 4 +++-\n git-mergetool--lib.sh | 27 ++++++++++++++++++---------\n t/t7610-mergetool.sh | 19 +++++++++++++++++++\n 3 files changed, 40 insertions(+), 10 deletions(-)\n\ndiff --git a/Documentation/git-mergetool.txt b/Documentation/git-mergetool.txt\nindex 0c7975a050..6b14702e78 100644\n--- a/Documentation/git-mergetool.txt\n+++ b/Documentation/git-mergetool.txt\n@@ -83,7 +83,9 @@ success of the resolution after the custom tool has exited.\n --gui::\n \tWhen 'git-mergetool' is invoked with the `-g` or `--gui` option\n \tthe default merge tool will be read from the configured\n-\t`merge.guitool` variable instead of `merge.tool`.\n+\t`merge.guitool` variable instead of `merge.tool`. If\n+\t`merge.guitool` is not set, we will fallback to the tool\n+\tconfigured under `merge.tool`.\n \n --no-gui::\n \tThis overrides a previous `-g` or `--gui` setting and reads the\ndiff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh\nindex d5e2c6c5c6..68a85f4a7b 100644\n--- a/git-mergetool--lib.sh\n+++ b/git-mergetool--lib.sh\n@@ -350,20 +350,29 @@ guess_merge_tool () {\n }\n \n get_configured_merge_tool () {\n-\t# If first argument is true, find the guitool instead\n-\tif test \"$1\" = true\n+\tis_gui=\"$1\"\n+\tsections=\"merge\"\n+\tkeys=\"tool\"\n+\n+\tif diff_mode\n \tthen\n-\t\tgui_prefix=gui\n+\t\tsections=\"diff $sections\"\n \tfi\n \n-\t# Diff mode first tries diff.(gui)tool and falls back to merge.(gui)tool.\n-\t# Merge mode only checks merge.(gui)tool\n-\tif diff_mode\n+\tif \"$is_gui\" = true\n \tthen\n-\t\tmerge_tool=$(git config diff.${gui_prefix}tool || git config merge.${gui_prefix}tool)\n-\telse\n-\t\tmerge_tool=$(git config merge.${gui_prefix}tool)\n+\t\tkeys=\"guitool $keys\"\n \tfi\n+\n+\tIFS=' '\n+\tfor key in $keys\n+\tdo\n+\t\tfor section in $sections\n+\t\tdo\n+\t\t\tmerge_tool=$(git config $section.$key) && break 2\n+\t\tdone\n+\tdone\n+\n \tif test -n \"$merge_tool\" && ! valid_tool \"$merge_tool\"\n \tthen\n \t\techo >&2 \"git config option $TOOL_MODE.${gui_prefix}tool set to unknown tool: $merge_tool\"\ndiff --git a/t/t7610-mergetool.sh b/t/t7610-mergetool.sh\nindex 5f37d7a1ff..bc2c9eaa30 100755\n--- a/t/t7610-mergetool.sh\n+++ b/t/t7610-mergetool.sh\n@@ -167,6 +167,25 @@ test_expect_success 'gui mergetool' '\n \tgit commit -m \"branch1 resolved with mergetool\"\n '\n \n+test_expect_success 'gui mergetool without merge.guitool set fallsback to merge.tool' '\n+\ttest_when_finished \"git reset --hard\" &&\n+\tgit checkout -b test$test_count branch1 &&\n+\tgit submodule update -N &&\n+\ttest_must_fail git merge master >/dev/null 2>&1 &&\n+\t( yes \"\" | git mergetool --gui both >/dev/null 2>&1 ) &&\n+\t( yes \"\" | git mergetool -g file1 file1 ) &&\n+\t( yes \"\" | git mergetool --gui file2 \"spaced name\" >/dev/null 2>&1 ) &&\n+\t( yes \"\" | git mergetool --gui subdir/file3 >/dev/null 2>&1 ) &&\n+\t( yes \"d\" | git mergetool --gui file11 >/dev/null 2>&1 ) &&\n+\t( yes \"d\" | git mergetool --gui file12 >/dev/null 2>&1 ) &&\n+\t( yes \"l\" | git mergetool --gui submod >/dev/null 2>&1 ) &&\n+\ttest \"$(cat file1)\" = \"master updated\" &&\n+\ttest \"$(cat file2)\" = \"master new\" &&\n+\ttest \"$(cat subdir/file3)\" = \"master new sub\" &&\n+\ttest \"$(cat submod/bar)\" = \"branch1 submodule\" &&\n+\tgit commit -m \"branch1 resolved with mergetool\"\n+'\n+\n test_expect_success 'mergetool crlf' '\n \ttest_when_finished \"git reset --hard\" &&\n \t# This test_config line must go after the above reset line so that\n-- \n2.21.0.967.gf85e14fd49\n\n"},{"id":"374221","messageId":"c019926b32016369e9f497d8e227107fdf192440.1555880168.git.liu.denton@gmail.com","threadId":"50966","inReplyTo":"cover.1555880168.git.liu.denton@gmail.com","subject":"[PATCH 4/5] difftool: make --gui, --tool and --extcmd exclusive","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2019-04-22T05:07:48Z","receivedAt":"2019-04-22T05:07:54Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"In git-difftool, these options specify which tool to ultimately run. As\na result, they are logically conflicting. Explicitly disallow these\noptions from being used together.\n\nSigned-off-by: Denton Liu \n---\n builtin/difftool.c | 11 ++++++++++-\n t/t7800-difftool.sh | 8 ++++++++\n 2 files changed, 18 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/difftool.c b/builtin/difftool.c\nindex a3ea60ea71..5ad39c9172 100644\n--- a/builtin/difftool.c\n+++ b/builtin/difftool.c\n@@ -690,7 +690,7 @@ static int run_file_diff(int prompt, const char *prefix,\n int cmd_difftool(int argc, const char **argv, const char *prefix)\n {\n \tint use_gui_tool = 0, dir_diff = 0, prompt = -1, symlinks = 0,\n-\t tool_help = 0;\n+\t tool_help = 0, count = 0;\n \tstatic char *difftool_cmd = NULL, *extcmd = NULL;\n \tstruct option builtin_difftool_options[] = {\n \t\tOPT_BOOL('g', \"gui\", &use_gui_tool,\n@@ -731,6 +731,15 @@ int cmd_difftool(int argc, const char **argv, const char *prefix)\n \tsetenv(GIT_DIR_ENVIRONMENT, absolute_path(get_git_dir()), 1);\n \tsetenv(GIT_WORK_TREE_ENVIRONMENT, absolute_path(get_git_work_tree()), 1);\n \n+\tif (use_gui_tool)\n+\t\tcount++;\n+\tif (difftool_cmd)\n+\t\tcount++;\n+\tif (extcmd)\n+\t\tcount++;\n+\tif (count > 1)\n+\t\tdie(_(\"--gui, --tool and --extcmd are exclusive\"));\n+\n \tif (use_gui_tool && diff_gui_tool && *diff_gui_tool)\n \t\tsetenv(\"GIT_DIFF_TOOL\", diff_gui_tool, 1);\n \telse if (difftool_cmd) {\ndiff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh\nindex bb9a7f4ff9..107f31213d 100755\n--- a/t/t7800-difftool.sh\n+++ b/t/t7800-difftool.sh\n@@ -705,4 +705,12 @@ test_expect_success SYMLINKS 'difftool --dir-diff handles modified symlinks' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'difftool --gui, --tool and --extcmd are exclusive' '\n+\tdifftool_test_setup &&\n+\ttest_must_fail git difftool --gui --tool=test-tool &&\n+\ttest_must_fail git difftool --gui --extcmd=cat &&\n+\ttest_must_fail git difftool --tool=test-tool --extcmd=cat &&\n+\ttest_must_fail git difftool --gui --tool=test-tool --extcmd=cat\n+'\n+\n test_done\n-- \n2.21.0.967.gf85e14fd49\n\n"},{"id":"374222","messageId":"fb7ac11439cbfd52d9181b78fdc8f8034a6b1064.1555880168.git.liu.denton@gmail.com","threadId":"50966","inReplyTo":"cover.1555880168.git.liu.denton@gmail.com","subject":"[PATCH 5/5] difftool: fallback on merge.guitool","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2019-04-22T05:07:50Z","receivedAt":"2019-04-22T05:07:58Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"In git-difftool.txt, it says\n\n\t'git difftool' falls back to 'git mergetool' config variables when the\n\tdifftool equivalents have not been defined.\n\nHowever, when `diff.guitool` is missing, it doesn't fallback to\nanything. Make git-difftool fallback to `merge.guitool` when `diff.guitool` is\nmissing.\n\nSigned-off-by: Denton Liu \n---\n Documentation/git-difftool.txt | 4 +++-\n builtin/difftool.c | 10 ++--------\n t/t7800-difftool.sh | 16 ++++++++++++++++\n 3 files changed, 21 insertions(+), 9 deletions(-)\n\ndiff --git a/Documentation/git-difftool.txt b/Documentation/git-difftool.txt\nindex 96c26e6aa8..484c485fd0 100644\n--- a/Documentation/git-difftool.txt\n+++ b/Documentation/git-difftool.txt\n@@ -90,7 +90,9 @@ instead. `--no-symlinks` is the default on Windows.\n \tWhen 'git-difftool' is invoked with the `-g` or `--gui` option\n \tthe default diff tool will be read from the configured\n \t`diff.guitool` variable instead of `diff.tool`. The `--no-gui`\n-\toption can be used to override this setting.\n+\toption can be used to override this setting. If `diff.guitool`\n+\tis not set, we will fallback in the order of `merge.guitool`,\n+\t`diff.tool`, `merge.tool` until a tool is found.\n \n --[no-]trust-exit-code::\n \t'git-difftool' invokes a diff tool individually on each file.\ndiff --git a/builtin/difftool.c b/builtin/difftool.c\nindex 5ad39c9172..67f26502c5 100644\n--- a/builtin/difftool.c\n+++ b/builtin/difftool.c\n@@ -24,7 +24,6 @@\n #include \"object-store.h\"\n #include \"dir.h\"\n \n-static char *diff_gui_tool;\n static int trust_exit_code;\n \n static const char *const builtin_difftool_usage[] = {\n@@ -34,11 +33,6 @@ static const char *const builtin_difftool_usage[] = {\n \n static int difftool_config(const char *var, const char *value, void *cb)\n {\n-\tif (!strcmp(var, \"diff.guitool\")) {\n-\t\tdiff_gui_tool = xstrdup(value);\n-\t\treturn 0;\n-\t}\n-\n \tif (!strcmp(var, \"difftool.trustexitcode\")) {\n \t\ttrust_exit_code = git_config_bool(var, value);\n \t\treturn 0;\n@@ -740,8 +734,8 @@ int cmd_difftool(int argc, const char **argv, const char *prefix)\n \tif (count > 1)\n \t\tdie(_(\"--gui, --tool and --extcmd are exclusive\"));\n \n-\tif (use_gui_tool && diff_gui_tool && *diff_gui_tool)\n-\t\tsetenv(\"GIT_DIFF_TOOL\", diff_gui_tool, 1);\n+\tif (use_gui_tool)\n+\t\tsetenv(\"GIT_MERGETOOL_GUI\", \"true\", 1);\n \telse if (difftool_cmd) {\n \t\tif (*difftool_cmd)\n \t\t\tsetenv(\"GIT_DIFF_TOOL\", difftool_cmd, 1);\ndiff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh\nindex 107f31213d..ae90701a12 100755\n--- a/t/t7800-difftool.sh\n+++ b/t/t7800-difftool.sh\n@@ -279,11 +279,27 @@ test_expect_success 'difftool + mergetool config variables' '\n \techo branch >expect &&\n \tgit difftool --no-prompt branch >actual &&\n \ttest_cmp expect actual &&\n+\tgit difftool --gui --no-prompt branch >actual &&\n+\ttest_cmp expect actual &&\n \n \t# set merge.tool to something bogus, diff.tool to test-tool\n \ttest_config merge.tool bogus-tool &&\n \ttest_config diff.tool test-tool &&\n \tgit difftool --no-prompt branch >actual &&\n+\ttest_cmp expect actual &&\n+\tgit difftool --gui --no-prompt branch >actual &&\n+\ttest_cmp expect actual &&\n+\n+\t# set merge.tool, diff.tool to something bogus, merge.guitool to test-tool\n+\ttest_config diff.tool bogus-tool &&\n+\ttest_config merge.guitool test-tool &&\n+\tgit difftool --gui --no-prompt branch >actual &&\n+\ttest_cmp expect actual &&\n+\n+\t# set merge.tool, diff.tool, merge.guitool to something bogus, diff.guitool to test-tool\n+\ttest_config merge.guitool bogus-tool &&\n+\ttest_config diff.guitool test-tool &&\n+\tgit difftool --gui --no-prompt branch >actual &&\n \ttest_cmp expect actual\n '\n \n-- \n2.21.0.967.gf85e14fd49\n\n"},{"id":"374230","messageId":"CAPig+cQQqLc16=6iO=z6bBDMKD_dnG0ZL=7_onGibfYxoJsN=g@mail.gmail.com","threadId":"50966","inReplyTo":"c019926b32016369e9f497d8e227107fdf192440.1555880168.git.liu.denton@gmail.com","subject":"Re: [PATCH 4/5] difftool: make --gui, --tool and --extcmd exclusive","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2019-04-22T07:03:51Z","receivedAt":"2019-04-22T07:04:05Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Mon, Apr 22, 2019 at 1:07 AM Denton Liu wrote:\n> In git-difftool, these options specify which tool to ultimately run. As\n> a result, they are logically conflicting. Explicitly disallow these\n> options from being used together.\n>\n> Signed-off-by: Denton Liu \n> ---\n> diff --git a/builtin/difftool.c b/builtin/difftool.c\n> @@ -731,6 +731,15 @@ int cmd_difftool(int argc, const char **argv, const char *prefix)\n> + if (use_gui_tool)\n> + count++;\n> + if (difftool_cmd)\n> + count++;\n> + if (extcmd)\n> + count++;\n> + if (count > 1)\n> + die(_(\"--gui, --tool and --extcmd are exclusive\"));\n\nThe more typical way to check this condition in this codebase is:\n\n if (use_gui_tool + !!difftool_cmd + !!extcmd > 1)\n die(...);\n\nAlso, I think you might want: s/exclusive/mutually &/\n"},{"id":"374231","messageId":"CAPig+cTJf_2psxrtsOi2h7c3ZbDj25WqLGPNaLqBmoVpM+6FRg@mail.gmail.com","threadId":"50966","inReplyTo":"692875cf4baaeee8b47fd7e95d0b787d1a08f64e.1555880168.git.liu.denton@gmail.com","subject":"Re: [PATCH 2/5] mergetool: use get_merge_tool function","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2019-04-22T07:07:25Z","receivedAt":"2019-04-22T07:07:40Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Mon, Apr 22, 2019 at 1:07 AM Denton Liu wrote:\n> [...]\n> Rewrite `get_merge_tool` to return whether or not the tool was guessed\n> and make git-mergetool call this function instead of duplicating the\n> logic. Also, let `$GIT_MERGETOOL_GUI` be set to determine whether or not\n> the guitool will be selected.\n>\n> Signed-off-by: Denton Liu \n> ---\n> diff --git a/Documentation/git-mergetool--lib.txt b/Documentation/git-mergetool--lib.txt\n> @@ -28,7 +28,10 @@ to define the operation mode for the functions listed below.\n> get_merge_tool::\n> - returns a merge tool.\n> + returns '$is_guessed:$merge_tool'. '$is_guessed' is 'true' if\n> + the tool was guessed, else 'false'. '$merge_tool' is the merge\n> + tool to use. '$GIT_MERGETOOL_GUI' may be set to 'true' to search\n> + for the appropriate guitool.\n\nWhat is the likelihood that code outside of our control is using this\nfunction? If there is such code, this backward-incompatible change\nwill break that code. If the likelihood is excessively small, perhaps\nit is not worth worrying about, otherwise, perhaps this warrants a new\nfunction with a distinct name.\n"},{"id":"374233","messageId":"20190422083517.GA12094@archbookpro.localdomain","threadId":"50966","inReplyTo":"CAPig+cTJf_2psxrtsOi2h7c3ZbDj25WqLGPNaLqBmoVpM+6FRg@mail.gmail.com","subject":"Re: [PATCH 2/5] mergetool: use get_merge_tool function","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2019-04-22T08:35:17Z","receivedAt":"2019-04-22T08:35:22Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"Hi Eric,\n\nOn Mon, Apr 22, 2019 at 03:07:25AM -0400, Eric Sunshine wrote:\n> On Mon, Apr 22, 2019 at 1:07 AM Denton Liu wrote:\n> > [...]\n> > Rewrite `get_merge_tool` to return whether or not the tool was guessed\n> > and make git-mergetool call this function instead of duplicating the\n> > logic. Also, let `$GIT_MERGETOOL_GUI` be set to determine whether or not\n> > the guitool will be selected.\n> >\n> > Signed-off-by: Denton Liu \n> > ---\n> > diff --git a/Documentation/git-mergetool--lib.txt b/Documentation/git-mergetool--lib.txt\n> > @@ -28,7 +28,10 @@ to define the operation mode for the functions listed below.\n> > get_merge_tool::\n> > - returns a merge tool.\n> > + returns '$is_guessed:$merge_tool'. '$is_guessed' is 'true' if\n> > + the tool was guessed, else 'false'. '$merge_tool' is the merge\n> > + tool to use. '$GIT_MERGETOOL_GUI' may be set to 'true' to search\n> > + for the appropriate guitool.\n> \n> What is the likelihood that code outside of our control is using this\n> function? If there is such code, this backward-incompatible change\n> will break that code. If the likelihood is excessively small, perhaps\n> it is not worth worrying about, otherwise, perhaps this warrants a new\n> function with a distinct name.\n\nThanks for considering this, I hadn't thought about it myself. I assumed\nthis was an internal function but I guess I was wrong.\n\nI did a bit of digging on GitHub and Google and I found that\ngit-diffall[1] uses it, although it seems quite old and unmaintained.\nAside from this, I can't find any other open-source programs which use\ngit-mergetool--lib (and in particular, get_merge_tool) so I believe that\nit is pretty rare.\n\nThat being said, I'm open to writing a new function so that the change\nwill be backwards compatible. I'll see what the list has to say.\n\nThanks,\n\nDenton\n\n[1]: https://github.com/thenigan/git-diffall\n"},{"id":"374259","messageId":"2a521a5c-4b5d-57aa-1e91-d4ec9b190fb7@jeffhostetler.com","threadId":"50966","inReplyTo":"fb7ac11439cbfd52d9181b78fdc8f8034a6b1064.1555880168.git.liu.denton@gmail.com","subject":"Re: [PATCH 5/5] difftool: fallback on merge.guitool","fromName":"Jeff Hostetler","fromEmail":"git@jeffhostetler.com","sentAt":"2019-04-22T18:18:36Z","receivedAt":"2019-04-22T18:18:41Z","isPatch":true,"sender":{"key":"git@jeffhostetler.com","avatar":null},"body":"\n\nOn 4/22/2019 1:07 AM, Denton Liu wrote:\n> In git-difftool.txt, it says\n> \n> \t'git difftool' falls back to 'git mergetool' config variables when the\n> \tdifftool equivalents have not been defined.\n> \n> However, when `diff.guitool` is missing, it doesn't fallback to\n> anything. Make git-difftool fallback to `merge.guitool` when `diff.guitool` is\n> missing.\n> \n\nIs this a well-defined operation?\n\nI mean, we're assuming here that a 3-way gui merge tool (that probably\nexpects 3 input pathnames and maybe a 4th merge-result pathname (and\nassociated titles and etc)) can function sanely when only given the\npair that a diff would have.\n\nThat is, we're assuming that the selected merge tool has a 2-way diff\nmode and that the command line args for the 2- and 3-way views are\ncompatible.\n\nJust a thought\nJeff\n\n\n"},{"id":"374261","messageId":"20190422183356.GA6730@dev-l","threadId":"50966","inReplyTo":"2a521a5c-4b5d-57aa-1e91-d4ec9b190fb7@jeffhostetler.com","subject":"Re: [PATCH 5/5] difftool: fallback on merge.guitool","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2019-04-22T18:33:56Z","receivedAt":"2019-04-22T18:34:01Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"Hi Jeff,\n\nOn Mon, Apr 22, 2019 at 02:18:36PM -0400, Jeff Hostetler wrote:\n> \n> \n> On 4/22/2019 1:07 AM, Denton Liu wrote:\n> >In git-difftool.txt, it says\n> >\n> >\t'git difftool' falls back to 'git mergetool' config variables when the\n> >\tdifftool equivalents have not been defined.\n> >\n> >However, when `diff.guitool` is missing, it doesn't fallback to\n> >anything. Make git-difftool fallback to `merge.guitool` when `diff.guitool` is\n> >missing.\n> >\n> \n> Is this a well-defined operation?\n\nI believe this is a yes.\n\n> \n> I mean, we're assuming here that a 3-way gui merge tool (that probably\n> expects 3 input pathnames and maybe a 4th merge-result pathname (and\n> associated titles and etc)) can function sanely when only given the\n> pair that a diff would have.\n> \n> That is, we're assuming that the selected merge tool has a 2-way diff\n> mode and that the command line args for the 2- and 3-way views are\n> compatible.\n\nIf I read the code correctly, it seems like the only tool that is\nstrictly \"merge-only\" is tortoisemerge. In mergetools/tortoisemerge, we\nhave\n\n\tcan_diff () {\n\t\treturn 1\n\t}\n\nwhich means it will refuse to run as a difftool.\n\nIn the case where it fails like this, it'll loudly complain to the user,\nwhich'll give them a chance to fix their config. I believe that this is\ndesired behaviour and the patch adds on top of that.\n\nThanks,\n\nDenton\n\n> \n> Just a thought\n> Jeff\n> \n> \n"},{"id":"374305","messageId":"cover.1556009181.git.liu.denton@gmail.com","threadId":"50966","inReplyTo":"cover.1555880168.git.liu.denton@gmail.com","subject":"[PATCH v2 0/5] difftool and mergetool improvements","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2019-04-23T08:53:55Z","receivedAt":"2019-04-23T08:54:02Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"Thanks for the review, Eric.\n\nHopefully, these changes have addressed the concerns that you've raised.\n\n---\n\nChanges since v1:\n\n* Introduce get_merge_tool_guessed function instead of changing\n get_merge_tool\n* Remove unnecessary if-tower in mutual exclusivity logic\n\nDenton Liu (5):\n t7610: add mergetool --gui tests\n mergetool: use get_merge_tool_guessed function\n mergetool: fallback to tool when guitool unavailable\n difftool: make --gui, --tool and --extcmd mutually exclusive\n difftool: fallback on merge.guitool\n\n Documentation/git-difftool.txt | 4 ++-\n Documentation/git-mergetool--lib.txt | 9 +++++-\n Documentation/git-mergetool.txt | 4 ++-\n builtin/difftool.c | 13 ++++-----\n git-mergetool--lib.sh | 39 ++++++++++++++++++--------\n git-mergetool.sh | 11 ++------\n t/t7610-mergetool.sh | 41 ++++++++++++++++++++++++++++\n t/t7800-difftool.sh | 24 ++++++++++++++++\n 8 files changed, 114 insertions(+), 31 deletions(-)\n\nRange-diff against v1:\n1: 678f9b11fc = 1: 678f9b11fc t7610: add mergetool --gui tests\n2: 692875cf4b ! 2: e928db892e mergetool: use get_merge_tool function\n @@ -1,15 +1,19 @@\n Author: Denton Liu \n \n - mergetool: use get_merge_tool function\n + mergetool: use get_merge_tool_guessed function\n \n In git-mergetool, the logic for getting which merge tool to use is\n duplicated in git-mergetool--lib, except for the fact that it needs to\n know whether the tool was guessed or not.\n \n - Rewrite `get_merge_tool` to return whether or not the tool was guessed\n - and make git-mergetool call this function instead of duplicating the\n - logic. Also, let `$GIT_MERGETOOL_GUI` be set to determine whether or not\n - the guitool will be selected.\n + Write `get_merge_tool_guessed` to return whether or not the tool was\n + guessed in addition to the actual tool and make git-mergetool call this\n + function instead of duplicating the logic. Also, let\n + `$GIT_MERGETOOL_GUI` be set to determine whether or not the guitool will\n + be selected.\n +\n + Make `get_merge_tool` use this function internally so that code\n + duplication is reduced.\n \n Signed-off-by: Denton Liu \n \n @@ -17,38 +21,32 @@\n --- a/Documentation/git-mergetool--lib.txt\n +++ b/Documentation/git-mergetool--lib.txt\n @@\n + \n FUNCTIONS\n ---------\n - get_merge_tool::\n --\treturns a merge tool.\n ++get_merge_tool_guessed::\n +\treturns '$is_guessed:$merge_tool'. '$is_guessed' is 'true' if\n +\tthe tool was guessed, else 'false'. '$merge_tool' is the merge\n +\ttool to use. '$GIT_MERGETOOL_GUI' may be set to 'true' to search\n +\tfor the appropriate guitool.\n ++\n + get_merge_tool::\n +-\treturns a merge tool.\n ++\treturns a merge tool. '$GIT_MERGETOOL_GUI' may be set to 'true'\n ++\tto search for the appropriate guitool.\n \n get_merge_tool_cmd::\n \treturns the custom command for a merge tool.\n \n - diff --git a/git-difftool--helper.sh b/git-difftool--helper.sh\n - --- a/git-difftool--helper.sh\n - +++ b/git-difftool--helper.sh\n -@@\n - \tthen\n - \t\tmerge_tool=\"$GIT_DIFF_TOOL\"\n - \telse\n --\t\tmerge_tool=\"$(get_merge_tool)\" || exit\n -+\t\tmerge_tool=\"$(get_merge_tool | sed -e 's/^[a-z]*://')\" || exit\n - \tfi\n - fi\n - \n -\n diff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh\n --- a/git-mergetool--lib.sh\n +++ b/git-mergetool--lib.sh\n @@\n + \techo \"$merge_tool_path\"\n }\n \n - get_merge_tool () {\n +-get_merge_tool () {\n ++get_merge_tool_guessed () {\n +\tis_guessed=false\n \t# Check if a merge tool has been configured\n -\tmerge_tool=$(get_configured_merge_tool)\n @@ -61,6 +59,10 @@\n \tfi\n -\techo \"$merge_tool\"\n +\techo \"$is_guessed:$merge_tool\"\n ++}\n ++\n ++get_merge_tool () {\n ++\tget_merge_tool_guessed | sed -e 's/^[a-z]*://'\n }\n \n mergetool_find_win32_cmd () {\n @@ -81,7 +83,7 @@\n -\t\t\tguessed_merge_tool=true\n -\t\tfi\n +\t\tIFS=':' read guessed_merge_tool merge_tool <<-EOF\n -+\t\t$(GIT_MERGETOOL_GUI=$gui_tool get_merge_tool)\n ++\t\t$(GIT_MERGETOOL_GUI=$gui_tool get_merge_tool_guessed)\n +\t\tEOF\n \tfi\n \tmerge_keep_backup=\"$(git config --bool mergetool.keepBackup || echo true)\"\n3: de1b897a11 = 3: 24db1afeee mergetool: fallback to tool when guitool unavailable\n4: a272594bd2 = 4: 6f65b5c913 difftool: make --gui, --tool and --extcmd mutually exclusive\n5: 4fc3f84bad = 5: 5a24772219 difftool: fallback on merge.guitool\n-- \n2.21.0.1000.g11cd861522\n\n"},{"id":"374306","messageId":"678f9b11fc7df7d3ee2050388574bcaea86af331.1556009181.git.liu.denton@gmail.com","threadId":"50966","inReplyTo":"cover.1556009181.git.liu.denton@gmail.com","subject":"[PATCH v2 1/5] t7610: add mergetool --gui tests","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2019-04-23T08:53:58Z","receivedAt":"2019-04-23T08:54:04Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"In 063f2bdbf7 (mergetool: accept -g/--[no-]gui as arguments,\n2018-10-24), mergetool was taught the --gui option but no tests were\nadded to ensure that it was working properly. Add a test to ensure that\nit works.\n\nSigned-off-by: Denton Liu \n---\n t/t7610-mergetool.sh | 22 ++++++++++++++++++++++\n 1 file changed, 22 insertions(+)\n\ndiff --git a/t/t7610-mergetool.sh b/t/t7610-mergetool.sh\nindex a9fb971615..5f37d7a1ff 100755\n--- a/t/t7610-mergetool.sh\n+++ b/t/t7610-mergetool.sh\n@@ -145,6 +145,28 @@ test_expect_success 'custom mergetool' '\n \tgit commit -m \"branch1 resolved with mergetool\"\n '\n \n+test_expect_success 'gui mergetool' '\n+\ttest_config merge.guitool myguitool &&\n+\ttest_config mergetool.myguitool.cmd \"(printf \\\"gui \\\" && cat \\\"\\$REMOTE\\\") >\\\"\\$MERGED\\\"\" &&\n+\ttest_config mergetool.myguitool.trustExitCode true &&\n+\ttest_when_finished \"git reset --hard\" &&\n+\tgit checkout -b test$test_count branch1 &&\n+\tgit submodule update -N &&\n+\ttest_must_fail git merge master >/dev/null 2>&1 &&\n+\t( yes \"\" | git mergetool --gui both >/dev/null 2>&1 ) &&\n+\t( yes \"\" | git mergetool -g file1 file1 ) &&\n+\t( yes \"\" | git mergetool --gui file2 \"spaced name\" >/dev/null 2>&1 ) &&\n+\t( yes \"\" | git mergetool --gui subdir/file3 >/dev/null 2>&1 ) &&\n+\t( yes \"d\" | git mergetool --gui file11 >/dev/null 2>&1 ) &&\n+\t( yes \"d\" | git mergetool --gui file12 >/dev/null 2>&1 ) &&\n+\t( yes \"l\" | git mergetool --gui submod >/dev/null 2>&1 ) &&\n+\ttest \"$(cat file1)\" = \"gui master updated\" &&\n+\ttest \"$(cat file2)\" = \"gui master new\" &&\n+\ttest \"$(cat subdir/file3)\" = \"gui master new sub\" &&\n+\ttest \"$(cat submod/bar)\" = \"branch1 submodule\" &&\n+\tgit commit -m \"branch1 resolved with mergetool\"\n+'\n+\n test_expect_success 'mergetool crlf' '\n \ttest_when_finished \"git reset --hard\" &&\n \t# This test_config line must go after the above reset line so that\n-- \n2.21.0.1000.g11cd861522\n\n"},{"id":"374307","messageId":"e928db892e35bcb68fcdb52c9bf7158dbbb46616.1556009181.git.liu.denton@gmail.com","threadId":"50966","inReplyTo":"cover.1556009181.git.liu.denton@gmail.com","subject":"[PATCH v2 2/5] mergetool: use get_merge_tool_guessed function","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2019-04-23T08:54:01Z","receivedAt":"2019-04-23T08:54:06Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"In git-mergetool, the logic for getting which merge tool to use is\nduplicated in git-mergetool--lib, except for the fact that it needs to\nknow whether the tool was guessed or not.\n\nWrite `get_merge_tool_guessed` to return whether or not the tool was\nguessed in addition to the actual tool and make git-mergetool call this\nfunction instead of duplicating the logic. Also, let\n`$GIT_MERGETOOL_GUI` be set to determine whether or not the guitool will\nbe selected.\n\nMake `get_merge_tool` use this function internally so that code\nduplication is reduced.\n\nSigned-off-by: Denton Liu \n---\n\nAfter thinking about it for a while, I realised that if it was easy to\nfind one (albeit old) public project using our code, there should be\nmany others who we don't know about that will also be using our code.\n\nLet's save them the trouble and just introduce a new function instead of\nchanging the behaviour of the old one.\n\n---\n Documentation/git-mergetool--lib.txt | 9 ++++++++-\n git-mergetool--lib.sh | 12 +++++++++---\n git-mergetool.sh | 11 +++--------\n 3 files changed, 20 insertions(+), 12 deletions(-)\n\ndiff --git a/Documentation/git-mergetool--lib.txt b/Documentation/git-mergetool--lib.txt\nindex 055550b2bc..343268d885 100644\n--- a/Documentation/git-mergetool--lib.txt\n+++ b/Documentation/git-mergetool--lib.txt\n@@ -27,8 +27,15 @@ to define the operation mode for the functions listed below.\n \n FUNCTIONS\n ---------\n+get_merge_tool_guessed::\n+\treturns '$is_guessed:$merge_tool'. '$is_guessed' is 'true' if\n+\tthe tool was guessed, else 'false'. '$merge_tool' is the merge\n+\ttool to use. '$GIT_MERGETOOL_GUI' may be set to 'true' to search\n+\tfor the appropriate guitool.\n+\n get_merge_tool::\n-\treturns a merge tool.\n+\treturns a merge tool. '$GIT_MERGETOOL_GUI' may be set to 'true'\n+\tto search for the appropriate guitool.\n \n get_merge_tool_cmd::\n \treturns the custom command for a merge tool.\ndiff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh\nindex 83bf52494c..5eedb1a08a 100644\n--- a/git-mergetool--lib.sh\n+++ b/git-mergetool--lib.sh\n@@ -402,15 +402,21 @@ get_merge_tool_path () {\n \techo \"$merge_tool_path\"\n }\n \n-get_merge_tool () {\n+get_merge_tool_guessed () {\n+\tis_guessed=false\n \t# Check if a merge tool has been configured\n-\tmerge_tool=$(get_configured_merge_tool)\n+\tmerge_tool=$(get_configured_merge_tool $GIT_MERGETOOL_GUI)\n \t# Try to guess an appropriate merge tool if no tool has been set.\n \tif test -z \"$merge_tool\"\n \tthen\n \t\tmerge_tool=$(guess_merge_tool) || exit\n+\t\tis_guessed=true\n \tfi\n-\techo \"$merge_tool\"\n+\techo \"$is_guessed:$merge_tool\"\n+}\n+\n+get_merge_tool () {\n+\tget_merge_tool_guessed | sed -e 's/^[a-z]*://'\n }\n \n mergetool_find_win32_cmd () {\ndiff --git a/git-mergetool.sh b/git-mergetool.sh\nindex 01b9ad59b2..63e4da1b2f 100755\n--- a/git-mergetool.sh\n+++ b/git-mergetool.sh\n@@ -449,14 +449,9 @@ main () {\n \n \tif test -z \"$merge_tool\"\n \tthen\n-\t\t# Check if a merge tool has been configured\n-\t\tmerge_tool=$(get_configured_merge_tool $gui_tool)\n-\t\t# Try to guess an appropriate merge tool if no tool has been set.\n-\t\tif test -z \"$merge_tool\"\n-\t\tthen\n-\t\t\tmerge_tool=$(guess_merge_tool) || exit\n-\t\t\tguessed_merge_tool=true\n-\t\tfi\n+\t\tIFS=':' read guessed_merge_tool merge_tool <<-EOF\n+\t\t$(GIT_MERGETOOL_GUI=$gui_tool get_merge_tool_guessed)\n+\t\tEOF\n \tfi\n \tmerge_keep_backup=\"$(git config --bool mergetool.keepBackup || echo true)\"\n \tmerge_keep_temporaries=\"$(git config --bool mergetool.keepTemporaries || echo false)\"\n-- \n2.21.0.1000.g11cd861522\n\n"},{"id":"374308","messageId":"24db1afeeee66894172cceaad9ffa170b75c62ff.1556009181.git.liu.denton@gmail.com","threadId":"50966","inReplyTo":"cover.1556009181.git.liu.denton@gmail.com","subject":"[PATCH v2 3/5] mergetool: fallback to tool when guitool unavailable","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2019-04-23T08:54:04Z","receivedAt":"2019-04-23T08:54:08Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"In git-difftool, if the tool is called with --gui but `diff.guitool` is\nnot set, it falls back to `diff.tool`. Make git-mergetool also fallback\nfrom `merge.guitool` to `merge.tool` if the former is undefined.\n\nIf git-difftool were to use `get_configured_mergetool`, it would also\nget the fallback behaviour in the following precedence:\n\n1. diff.guitool\n2. merge.guitool\n3. diff.tool\n4. merge.tool\n\nSigned-off-by: Denton Liu \n---\n Documentation/git-mergetool.txt | 4 +++-\n git-mergetool--lib.sh | 27 ++++++++++++++++++---------\n t/t7610-mergetool.sh | 19 +++++++++++++++++++\n 3 files changed, 40 insertions(+), 10 deletions(-)\n\ndiff --git a/Documentation/git-mergetool.txt b/Documentation/git-mergetool.txt\nindex 0c7975a050..6b14702e78 100644\n--- a/Documentation/git-mergetool.txt\n+++ b/Documentation/git-mergetool.txt\n@@ -83,7 +83,9 @@ success of the resolution after the custom tool has exited.\n --gui::\n \tWhen 'git-mergetool' is invoked with the `-g` or `--gui` option\n \tthe default merge tool will be read from the configured\n-\t`merge.guitool` variable instead of `merge.tool`.\n+\t`merge.guitool` variable instead of `merge.tool`. If\n+\t`merge.guitool` is not set, we will fallback to the tool\n+\tconfigured under `merge.tool`.\n \n --no-gui::\n \tThis overrides a previous `-g` or `--gui` setting and reads the\ndiff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh\nindex 5eedb1a08a..6f477ae77d 100644\n--- a/git-mergetool--lib.sh\n+++ b/git-mergetool--lib.sh\n@@ -350,20 +350,29 @@ guess_merge_tool () {\n }\n \n get_configured_merge_tool () {\n-\t# If first argument is true, find the guitool instead\n-\tif test \"$1\" = true\n+\tis_gui=\"$1\"\n+\tsections=\"merge\"\n+\tkeys=\"tool\"\n+\n+\tif diff_mode\n \tthen\n-\t\tgui_prefix=gui\n+\t\tsections=\"diff $sections\"\n \tfi\n \n-\t# Diff mode first tries diff.(gui)tool and falls back to merge.(gui)tool.\n-\t# Merge mode only checks merge.(gui)tool\n-\tif diff_mode\n+\tif \"$is_gui\" = true\n \tthen\n-\t\tmerge_tool=$(git config diff.${gui_prefix}tool || git config merge.${gui_prefix}tool)\n-\telse\n-\t\tmerge_tool=$(git config merge.${gui_prefix}tool)\n+\t\tkeys=\"guitool $keys\"\n \tfi\n+\n+\tIFS=' '\n+\tfor key in $keys\n+\tdo\n+\t\tfor section in $sections\n+\t\tdo\n+\t\t\tmerge_tool=$(git config $section.$key) && break 2\n+\t\tdone\n+\tdone\n+\n \tif test -n \"$merge_tool\" && ! valid_tool \"$merge_tool\"\n \tthen\n \t\techo >&2 \"git config option $TOOL_MODE.${gui_prefix}tool set to unknown tool: $merge_tool\"\ndiff --git a/t/t7610-mergetool.sh b/t/t7610-mergetool.sh\nindex 5f37d7a1ff..bc2c9eaa30 100755\n--- a/t/t7610-mergetool.sh\n+++ b/t/t7610-mergetool.sh\n@@ -167,6 +167,25 @@ test_expect_success 'gui mergetool' '\n \tgit commit -m \"branch1 resolved with mergetool\"\n '\n \n+test_expect_success 'gui mergetool without merge.guitool set fallsback to merge.tool' '\n+\ttest_when_finished \"git reset --hard\" &&\n+\tgit checkout -b test$test_count branch1 &&\n+\tgit submodule update -N &&\n+\ttest_must_fail git merge master >/dev/null 2>&1 &&\n+\t( yes \"\" | git mergetool --gui both >/dev/null 2>&1 ) &&\n+\t( yes \"\" | git mergetool -g file1 file1 ) &&\n+\t( yes \"\" | git mergetool --gui file2 \"spaced name\" >/dev/null 2>&1 ) &&\n+\t( yes \"\" | git mergetool --gui subdir/file3 >/dev/null 2>&1 ) &&\n+\t( yes \"d\" | git mergetool --gui file11 >/dev/null 2>&1 ) &&\n+\t( yes \"d\" | git mergetool --gui file12 >/dev/null 2>&1 ) &&\n+\t( yes \"l\" | git mergetool --gui submod >/dev/null 2>&1 ) &&\n+\ttest \"$(cat file1)\" = \"master updated\" &&\n+\ttest \"$(cat file2)\" = \"master new\" &&\n+\ttest \"$(cat subdir/file3)\" = \"master new sub\" &&\n+\ttest \"$(cat submod/bar)\" = \"branch1 submodule\" &&\n+\tgit commit -m \"branch1 resolved with mergetool\"\n+'\n+\n test_expect_success 'mergetool crlf' '\n \ttest_when_finished \"git reset --hard\" &&\n \t# This test_config line must go after the above reset line so that\n-- \n2.21.0.1000.g11cd861522\n\n"},{"id":"374309","messageId":"6f65b5c9134639eabd1abaf32af95c228a58e465.1556009181.git.liu.denton@gmail.com","threadId":"50966","inReplyTo":"cover.1556009181.git.liu.denton@gmail.com","subject":"[PATCH v2 4/5] difftool: make --gui, --tool and --extcmd mutually exclusive","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2019-04-23T08:54:06Z","receivedAt":"2019-04-23T08:54:11Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"In git-difftool, these options specify which tool to ultimately run. As\na result, they are logically conflicting. Explicitly disallow these\noptions from being used together.\n\nSigned-off-by: Denton Liu \n---\n builtin/difftool.c | 3 +++\n t/t7800-difftool.sh | 8 ++++++++\n 2 files changed, 11 insertions(+)\n\ndiff --git a/builtin/difftool.c b/builtin/difftool.c\nindex a3ea60ea71..65bba90338 100644\n--- a/builtin/difftool.c\n+++ b/builtin/difftool.c\n@@ -731,6 +731,9 @@ int cmd_difftool(int argc, const char **argv, const char *prefix)\n \tsetenv(GIT_DIR_ENVIRONMENT, absolute_path(get_git_dir()), 1);\n \tsetenv(GIT_WORK_TREE_ENVIRONMENT, absolute_path(get_git_work_tree()), 1);\n \n+\tif (use_gui_tool + !!difftool_cmd + !!extcmd > 1)\n+\t\tdie(_(\"--gui, --tool and --extcmd are mutually exclusive\"));\n+\n \tif (use_gui_tool && diff_gui_tool && *diff_gui_tool)\n \t\tsetenv(\"GIT_DIFF_TOOL\", diff_gui_tool, 1);\n \telse if (difftool_cmd) {\ndiff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh\nindex bb9a7f4ff9..107f31213d 100755\n--- a/t/t7800-difftool.sh\n+++ b/t/t7800-difftool.sh\n@@ -705,4 +705,12 @@ test_expect_success SYMLINKS 'difftool --dir-diff handles modified symlinks' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'difftool --gui, --tool and --extcmd are exclusive' '\n+\tdifftool_test_setup &&\n+\ttest_must_fail git difftool --gui --tool=test-tool &&\n+\ttest_must_fail git difftool --gui --extcmd=cat &&\n+\ttest_must_fail git difftool --tool=test-tool --extcmd=cat &&\n+\ttest_must_fail git difftool --gui --tool=test-tool --extcmd=cat\n+'\n+\n test_done\n-- \n2.21.0.1000.g11cd861522\n\n"},{"id":"374310","messageId":"5a24772219f405c93a54310d3e81a7c0e71c350e.1556009181.git.liu.denton@gmail.com","threadId":"50966","inReplyTo":"cover.1556009181.git.liu.denton@gmail.com","subject":"[PATCH v2 5/5] difftool: fallback on merge.guitool","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2019-04-23T08:54:09Z","receivedAt":"2019-04-23T08:54:14Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"In git-difftool.txt, it says\n\n\t'git difftool' falls back to 'git mergetool' config variables when the\n\tdifftool equivalents have not been defined.\n\nHowever, when `diff.guitool` is missing, it doesn't fallback to\nanything. Make git-difftool fallback to `merge.guitool` when `diff.guitool` is\nmissing.\n\nSigned-off-by: Denton Liu \n---\n Documentation/git-difftool.txt | 4 +++-\n builtin/difftool.c | 10 ++--------\n t/t7800-difftool.sh | 16 ++++++++++++++++\n 3 files changed, 21 insertions(+), 9 deletions(-)\n\ndiff --git a/Documentation/git-difftool.txt b/Documentation/git-difftool.txt\nindex 96c26e6aa8..484c485fd0 100644\n--- a/Documentation/git-difftool.txt\n+++ b/Documentation/git-difftool.txt\n@@ -90,7 +90,9 @@ instead. `--no-symlinks` is the default on Windows.\n \tWhen 'git-difftool' is invoked with the `-g` or `--gui` option\n \tthe default diff tool will be read from the configured\n \t`diff.guitool` variable instead of `diff.tool`. The `--no-gui`\n-\toption can be used to override this setting.\n+\toption can be used to override this setting. If `diff.guitool`\n+\tis not set, we will fallback in the order of `merge.guitool`,\n+\t`diff.tool`, `merge.tool` until a tool is found.\n \n --[no-]trust-exit-code::\n \t'git-difftool' invokes a diff tool individually on each file.\ndiff --git a/builtin/difftool.c b/builtin/difftool.c\nindex 65bba90338..10660639c0 100644\n--- a/builtin/difftool.c\n+++ b/builtin/difftool.c\n@@ -24,7 +24,6 @@\n #include \"object-store.h\"\n #include \"dir.h\"\n \n-static char *diff_gui_tool;\n static int trust_exit_code;\n \n static const char *const builtin_difftool_usage[] = {\n@@ -34,11 +33,6 @@ static const char *const builtin_difftool_usage[] = {\n \n static int difftool_config(const char *var, const char *value, void *cb)\n {\n-\tif (!strcmp(var, \"diff.guitool\")) {\n-\t\tdiff_gui_tool = xstrdup(value);\n-\t\treturn 0;\n-\t}\n-\n \tif (!strcmp(var, \"difftool.trustexitcode\")) {\n \t\ttrust_exit_code = git_config_bool(var, value);\n \t\treturn 0;\n@@ -734,8 +728,8 @@ int cmd_difftool(int argc, const char **argv, const char *prefix)\n \tif (use_gui_tool + !!difftool_cmd + !!extcmd > 1)\n \t\tdie(_(\"--gui, --tool and --extcmd are mutually exclusive\"));\n \n-\tif (use_gui_tool && diff_gui_tool && *diff_gui_tool)\n-\t\tsetenv(\"GIT_DIFF_TOOL\", diff_gui_tool, 1);\n+\tif (use_gui_tool)\n+\t\tsetenv(\"GIT_MERGETOOL_GUI\", \"true\", 1);\n \telse if (difftool_cmd) {\n \t\tif (*difftool_cmd)\n \t\t\tsetenv(\"GIT_DIFF_TOOL\", difftool_cmd, 1);\ndiff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh\nindex 107f31213d..ae90701a12 100755\n--- a/t/t7800-difftool.sh\n+++ b/t/t7800-difftool.sh\n@@ -279,11 +279,27 @@ test_expect_success 'difftool + mergetool config variables' '\n \techo branch >expect &&\n \tgit difftool --no-prompt branch >actual &&\n \ttest_cmp expect actual &&\n+\tgit difftool --gui --no-prompt branch >actual &&\n+\ttest_cmp expect actual &&\n \n \t# set merge.tool to something bogus, diff.tool to test-tool\n \ttest_config merge.tool bogus-tool &&\n \ttest_config diff.tool test-tool &&\n \tgit difftool --no-prompt branch >actual &&\n+\ttest_cmp expect actual &&\n+\tgit difftool --gui --no-prompt branch >actual &&\n+\ttest_cmp expect actual &&\n+\n+\t# set merge.tool, diff.tool to something bogus, merge.guitool to test-tool\n+\ttest_config diff.tool bogus-tool &&\n+\ttest_config merge.guitool test-tool &&\n+\tgit difftool --gui --no-prompt branch >actual &&\n+\ttest_cmp expect actual &&\n+\n+\t# set merge.tool, diff.tool, merge.guitool to something bogus, diff.guitool to test-tool\n+\ttest_config merge.guitool bogus-tool &&\n+\ttest_config diff.guitool test-tool &&\n+\tgit difftool --gui --no-prompt branch >actual &&\n \ttest_cmp expect actual\n '\n \n-- \n2.21.0.1000.g11cd861522\n\n"},{"id":"374370","messageId":"xmqq36m73mqj.fsf@gitster-ct.c.googlers.com","threadId":"50966","inReplyTo":"678f9b11fc7df7d3ee2050388574bcaea86af331.1556009181.git.liu.denton@gmail.com","subject":"Re: [PATCH v2 1/5] t7610: add mergetool --gui tests","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-04-24T07:07:16Z","receivedAt":"2019-04-24T07:07:24Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Denton Liu writes:\n\n> In 063f2bdbf7 (mergetool: accept -g/--[no-]gui as arguments,\n> 2018-10-24), mergetool was taught the --gui option but no tests were\n> added to ensure that it was working properly. Add a test to ensure that\n> it works.\n>\n> Signed-off-by: Denton Liu \n> ---\n> t/t7610-mergetool.sh | 22 ++++++++++++++++++++++\n> 1 file changed, 22 insertions(+)\n>\n> diff --git a/t/t7610-mergetool.sh b/t/t7610-mergetool.sh\n> index a9fb971615..5f37d7a1ff 100755\n> --- a/t/t7610-mergetool.sh\n> +++ b/t/t7610-mergetool.sh\n> @@ -145,6 +145,28 @@ test_expect_success 'custom mergetool' '\n> \tgit commit -m \"branch1 resolved with mergetool\"\n> '\n> \n> +test_expect_success 'gui mergetool' '\n> +\ttest_config merge.guitool myguitool &&\n> +\ttest_config mergetool.myguitool.cmd \"(printf \\\"gui \\\" && cat \\\"\\$REMOTE\\\") >\\\"\\$MERGED\\\"\" &&\n> +\ttest_config mergetool.myguitool.trustExitCode true &&\n> +\ttest_when_finished \"git reset --hard\" &&\n> +\tgit checkout -b test$test_count branch1 &&\n> +\tgit submodule update -N &&\n\n> +\ttest_must_fail git merge master >/dev/null 2>&1 &&\n> +\t( yes \"\" | git mergetool --gui both >/dev/null 2>&1 ) &&\n> +\t( yes \"\" | git mergetool -g file1 file1 ) &&\n> +\t( yes \"\" | git mergetool --gui file2 \"spaced name\" >/dev/null 2>&1 ) &&\n> +\t( yes \"\" | git mergetool --gui subdir/file3 >/dev/null 2>&1 ) &&\n> +\t( yes \"d\" | git mergetool --gui file11 >/dev/null 2>&1 ) &&\n> +\t( yes \"d\" | git mergetool --gui file12 >/dev/null 2>&1 ) &&\n> +\t( yes \"l\" | git mergetool --gui submod >/dev/null 2>&1 ) &&\n\nWe usually discourage suppressing the output from git commands being\ntested like the above via redirection. This new testlet seems to\nmimick the way some of the existing ones are written, but it seems\nthat not all invocations of mergetool in this file discard the\noutput. Is there a particular reason why there are two styles?\nIf not, I think we would want to standardize on *not* discarding.\n\nThanks.\n"},{"id":"374371","messageId":"xmqqr29r278v.fsf@gitster-ct.c.googlers.com","threadId":"50966","inReplyTo":"e928db892e35bcb68fcdb52c9bf7158dbbb46616.1556009181.git.liu.denton@gmail.com","subject":"Re: [PATCH v2 2/5] mergetool: use get_merge_tool_guessed function","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-04-24T07:27:12Z","receivedAt":"2019-04-24T07:27:24Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Denton Liu writes:\n\n> +get_merge_tool_guessed () {\n> +\tis_guessed=false\n> \t# Check if a merge tool has been configured\n> -\tmerge_tool=$(get_configured_merge_tool)\n> +\tmerge_tool=$(get_configured_merge_tool $GIT_MERGETOOL_GUI)\n> \t# Try to guess an appropriate merge tool if no tool has been set.\n> \tif test -z \"$merge_tool\"\n> \tthen\n> \t\tmerge_tool=$(guess_merge_tool) || exit\n> +\t\tis_guessed=true\n> \tfi\n> -\techo \"$merge_tool\"\n> +\techo \"$is_guessed:$merge_tool\"\n> +}\n> +\n> +get_merge_tool () {\n> +\tget_merge_tool_guessed | sed -e 's/^[a-z]*://'\n> }\n\nYuck. Returning a:b is fine if the main use is to match that string\nusing shell builtins like \"test\" and \"case\", but piping to \"sed\"\nfeels a bit too much overhead. Especially given that the other\nreader in git-emrgetool.sh is not protected for $merge_tool that has\na colon in it. Do not try to be too cute and end up with a hack\nthat is both inefficient and brittle at the same time.\n\nPossible alternatives:\n\n - Because variables in bourne family of shells are global, the\n caller can easily peek at $is_guessed; or\n\n - One bit \"did we guess, or did we get from the user?\" boolean\n choice can sufficiently be conveyed by ending the fuction like so\n instead:\n\n\t\t...\n\tfi\n\t\techo \"$merge_tool\"\n\t\ttest \"$is_guessed\" = true\n\t}\n\n> diff --git a/git-mergetool.sh b/git-mergetool.sh\n> index 01b9ad59b2..63e4da1b2f 100755\n> --- a/git-mergetool.sh\n> +++ b/git-mergetool.sh\n> @@ -449,14 +449,9 @@ main () {\n> \n> \tif test -z \"$merge_tool\"\n> \tthen\n> -\t\t# Check if a merge tool has been configured\n> -\t\tmerge_tool=$(get_configured_merge_tool $gui_tool)\n> -\t\t# Try to guess an appropriate merge tool if no tool has been set.\n> -\t\tif test -z \"$merge_tool\"\n> -\t\tthen\n> -\t\t\tmerge_tool=$(guess_merge_tool) || exit\n> -\t\t\tguessed_merge_tool=true\n> -\t\tfi\n> +\t\tIFS=':' read guessed_merge_tool merge_tool <<-EOF\n> +\t\t$(GIT_MERGETOOL_GUI=$gui_tool get_merge_tool_guessed)\n> +\t\tEOF\n\nWith the \"let the return code speak\" alternative, this would become\nsomething like\n\n\tif merge_tool=$(GIT_MERGETOOL_GUI=$gui_tool; get_merge_tool_guessed)\n\tthen\n\t\tguessed_merge_tool=true\n\telse\n\t\tguessed_merge_tool=false\n\tfi\n\t\nI do not know what you are trying with GIT_MERGETOOL_GUI=$gui_tool\nbefore the shell function, though. It does not work as one-shot\nassignment to an environment variable. I _think_ it is to feed the\nall-caps variable to get_configured_merge_tool that is invoked by\nthe get_merge_tool_guessed function, so it does not have to be\nexported as an environment in the first place, so in the above\nillustration, I simply wrote an assignment statement, followed by a\nseparate statement that is a parameterless call of a shell function,\nseparated by a semicolon.\n\n> \tfi\n> \tmerge_keep_backup=\"$(git config --bool mergetool.keepBackup || echo true)\"\n> \tmerge_keep_temporaries=\"$(git config --bool mergetool.keepTemporaries || echo false)\"\n"},{"id":"374429","messageId":"cover.1556142510.git.liu.denton@gmail.com","threadId":"50966","inReplyTo":"cover.1556009181.git.liu.denton@gmail.com","subject":"[PATCH v3 0/6] difftool and mergetool improvements","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2019-04-24T22:46:55Z","receivedAt":"2019-04-24T22:47:00Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"Thanks for the review, Junio.\n\nI hadn't thought about using the return code for `get_merge_tool` so\nthanks for that suggestion. It should be a lot cleaner now.\n\n---\n\nChanges since v2:\n\n* Unsuppress output in t7610\n* Make `get_merge_tool` return 1 on guessed so we don't have to deal\n with parsing '$guessed:$merge_tool'\n\nChanges since v1:\n\n* Introduce get_merge_tool_guessed function instead of changing\n get_merge_tool\n* Remove unnecessary if-tower in mutual exclusivity logic\n\nDenton Liu (6):\n t7610: unsuppress output\n t7610: add mergetool --gui tests\n mergetool: use get_merge_tool function\n mergetool: fallback to tool when guitool unavailable\n difftool: make --gui, --tool and --extcmd mutually exclusive\n difftool: fallback on merge.guitool\n\n Documentation/git-difftool.txt | 4 +-\n Documentation/git-mergetool--lib.txt | 4 +-\n Documentation/git-mergetool.txt | 4 +-\n builtin/difftool.c | 13 +--\n git-difftool--helper.sh | 2 +-\n git-mergetool--lib.sh | 32 ++++--\n git-mergetool.sh | 12 +-\n t/t7610-mergetool.sh | 163 +++++++++++++++++----------\n t/t7800-difftool.sh | 24 ++++\n 9 files changed, 167 insertions(+), 91 deletions(-)\n\nRange-diff against v2:\n-: ---------- > 1: 9f9922cab3 t7610: unsuppress output\n1: c436765684 ! 2: 0f632ca6bf t7610: add mergetool --gui tests\n @@ -23,14 +23,14 @@\n +\ttest_when_finished \"git reset --hard\" &&\n +\tgit checkout -b test$test_count branch1 &&\n +\tgit submodule update -N &&\n -+\ttest_must_fail git merge master >/dev/null 2>&1 &&\n -+\t( yes \"\" | git mergetool --gui both >/dev/null 2>&1 ) &&\n ++\ttest_must_fail git merge master &&\n ++\t( yes \"\" | git mergetool --gui both ) &&\n +\t( yes \"\" | git mergetool -g file1 file1 ) &&\n -+\t( yes \"\" | git mergetool --gui file2 \"spaced name\" >/dev/null 2>&1 ) &&\n -+\t( yes \"\" | git mergetool --gui subdir/file3 >/dev/null 2>&1 ) &&\n -+\t( yes \"d\" | git mergetool --gui file11 >/dev/null 2>&1 ) &&\n -+\t( yes \"d\" | git mergetool --gui file12 >/dev/null 2>&1 ) &&\n -+\t( yes \"l\" | git mergetool --gui submod >/dev/null 2>&1 ) &&\n ++\t( yes \"\" | git mergetool --gui file2 \"spaced name\" ) &&\n ++\t( yes \"\" | git mergetool --gui subdir/file3 ) &&\n ++\t( yes \"d\" | git mergetool --gui file11 ) &&\n ++\t( yes \"d\" | git mergetool --gui file12 ) &&\n ++\t( yes \"l\" | git mergetool --gui submod ) &&\n +\ttest \"$(cat file1)\" = \"gui master updated\" &&\n +\ttest \"$(cat file2)\" = \"gui master new\" &&\n +\ttest \"$(cat subdir/file3)\" = \"gui master new sub\" &&\n2: 9f8e76a421 < -: ---------- mergetool: use get_merge_tool_guessed function\n-: ---------- > 3: ff83d25ff2 mergetool: use get_merge_tool function\n3: 4847e64e46 ! 4: e975fe4a8b mergetool: fallback to tool when guitool unavailable\n @@ -81,18 +81,18 @@\n \tgit commit -m \"branch1 resolved with mergetool\"\n '\n \n -+test_expect_success 'gui mergetool without merge.guitool set fallsback to merge.tool' '\n ++test_expect_success 'gui mergetool without merge.guitool set falls back to merge.tool' '\n +\ttest_when_finished \"git reset --hard\" &&\n +\tgit checkout -b test$test_count branch1 &&\n +\tgit submodule update -N &&\n -+\ttest_must_fail git merge master >/dev/null 2>&1 &&\n -+\t( yes \"\" | git mergetool --gui both >/dev/null 2>&1 ) &&\n ++\ttest_must_fail git merge master &&\n ++\t( yes \"\" | git mergetool --gui both ) &&\n +\t( yes \"\" | git mergetool -g file1 file1 ) &&\n -+\t( yes \"\" | git mergetool --gui file2 \"spaced name\" >/dev/null 2>&1 ) &&\n -+\t( yes \"\" | git mergetool --gui subdir/file3 >/dev/null 2>&1 ) &&\n -+\t( yes \"d\" | git mergetool --gui file11 >/dev/null 2>&1 ) &&\n -+\t( yes \"d\" | git mergetool --gui file12 >/dev/null 2>&1 ) &&\n -+\t( yes \"l\" | git mergetool --gui submod >/dev/null 2>&1 ) &&\n ++\t( yes \"\" | git mergetool --gui file2 \"spaced name\" ) &&\n ++\t( yes \"\" | git mergetool --gui subdir/file3 ) &&\n ++\t( yes \"d\" | git mergetool --gui file11 ) &&\n ++\t( yes \"d\" | git mergetool --gui file12 ) &&\n ++\t( yes \"l\" | git mergetool --gui submod ) &&\n +\ttest \"$(cat file1)\" = \"master updated\" &&\n +\ttest \"$(cat file2)\" = \"master new\" &&\n +\ttest \"$(cat subdir/file3)\" = \"master new sub\" &&\n4: bc8cadaf55 = 5: bc3e229171 difftool: make --gui, --tool and --extcmd mutually exclusive\n5: e5e4dc3dd2 = 6: f39b15efbd difftool: fallback on merge.guitool\n-- \n2.21.0.1000.g7817e26e80\n\n"},{"id":"374430","messageId":"9f9922cab341396a846edc2f52342c59d294379a.1556142510.git.liu.denton@gmail.com","threadId":"50966","inReplyTo":"cover.1556142510.git.liu.denton@gmail.com","subject":"[PATCH v3 1/6] t7610: unsuppress output","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2019-04-24T22:46:57Z","receivedAt":"2019-04-24T22:47:02Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"The output for commands used to be suppressed by redirecting both stdout\nand stderr to /dev/null. However, this should not happen since the\noutput is useful for debugging and, without the \"-v\" flag, test scripts\ndon't output anyway.\n\nUnsuppress the output by removing the redirections to /dev/null.\n\nSigned-off-by: Denton Liu \n---\n t/t7610-mergetool.sh | 122 +++++++++++++++++++++----------------------\n 1 file changed, 61 insertions(+), 61 deletions(-)\n\ndiff --git a/t/t7610-mergetool.sh b/t/t7610-mergetool.sh\nindex a9fb971615..69711487dd 100755\n--- a/t/t7610-mergetool.sh\n+++ b/t/t7610-mergetool.sh\n@@ -130,14 +130,14 @@ test_expect_success 'custom mergetool' '\n \ttest_when_finished \"git reset --hard\" &&\n \tgit checkout -b test$test_count branch1 &&\n \tgit submodule update -N &&\n-\ttest_must_fail git merge master >/dev/null 2>&1 &&\n-\t( yes \"\" | git mergetool both >/dev/null 2>&1 ) &&\n+\ttest_must_fail git merge master &&\n+\t( yes \"\" | git mergetool both ) &&\n \t( yes \"\" | git mergetool file1 file1 ) &&\n-\t( yes \"\" | git mergetool file2 \"spaced name\" >/dev/null 2>&1 ) &&\n-\t( yes \"\" | git mergetool subdir/file3 >/dev/null 2>&1 ) &&\n-\t( yes \"d\" | git mergetool file11 >/dev/null 2>&1 ) &&\n-\t( yes \"d\" | git mergetool file12 >/dev/null 2>&1 ) &&\n-\t( yes \"l\" | git mergetool submod >/dev/null 2>&1 ) &&\n+\t( yes \"\" | git mergetool file2 \"spaced name\" ) &&\n+\t( yes \"\" | git mergetool subdir/file3 ) &&\n+\t( yes \"d\" | git mergetool file11 ) &&\n+\t( yes \"d\" | git mergetool file12 ) &&\n+\t( yes \"l\" | git mergetool submod ) &&\n \ttest \"$(cat file1)\" = \"master updated\" &&\n \ttest \"$(cat file2)\" = \"master new\" &&\n \ttest \"$(cat subdir/file3)\" = \"master new sub\" &&\n@@ -153,15 +153,15 @@ test_expect_success 'mergetool crlf' '\n \t# test_when_finished is LIFO.)\n \ttest_config core.autocrlf true &&\n \tgit checkout -b test$test_count branch1 &&\n-\ttest_must_fail git merge master >/dev/null 2>&1 &&\n-\t( yes \"\" | git mergetool file1 >/dev/null 2>&1 ) &&\n-\t( yes \"\" | git mergetool file2 >/dev/null 2>&1 ) &&\n-\t( yes \"\" | git mergetool \"spaced name\" >/dev/null 2>&1 ) &&\n-\t( yes \"\" | git mergetool both >/dev/null 2>&1 ) &&\n-\t( yes \"\" | git mergetool subdir/file3 >/dev/null 2>&1 ) &&\n-\t( yes \"d\" | git mergetool file11 >/dev/null 2>&1 ) &&\n-\t( yes \"d\" | git mergetool file12 >/dev/null 2>&1 ) &&\n-\t( yes \"r\" | git mergetool submod >/dev/null 2>&1 ) &&\n+\ttest_must_fail git merge master &&\n+\t( yes \"\" | git mergetool file1 ) &&\n+\t( yes \"\" | git mergetool file2 ) &&\n+\t( yes \"\" | git mergetool \"spaced name\" ) &&\n+\t( yes \"\" | git mergetool both ) &&\n+\t( yes \"\" | git mergetool subdir/file3 ) &&\n+\t( yes \"d\" | git mergetool file11 ) &&\n+\t( yes \"d\" | git mergetool file12 ) &&\n+\t( yes \"r\" | git mergetool submod ) &&\n \ttest \"$(printf x | cat file1 -)\" = \"$(printf \"master updated\\r\\nx\")\" &&\n \ttest \"$(printf x | cat file2 -)\" = \"$(printf \"master new\\r\\nx\")\" &&\n \ttest \"$(printf x | cat subdir/file3 -)\" = \"$(printf \"master new sub\\r\\nx\")\" &&\n@@ -176,8 +176,8 @@ test_expect_success 'mergetool in subdir' '\n \tgit submodule update -N &&\n \t(\n \t\tcd subdir &&\n-\t\ttest_must_fail git merge master >/dev/null 2>&1 &&\n-\t\t( yes \"\" | git mergetool file3 >/dev/null 2>&1 ) &&\n+\t\ttest_must_fail git merge master &&\n+\t\t( yes \"\" | git mergetool file3 ) &&\n \t\ttest \"$(cat file3)\" = \"master new sub\"\n \t)\n '\n@@ -188,14 +188,14 @@ test_expect_success 'mergetool on file in parent dir' '\n \tgit submodule update -N &&\n \t(\n \t\tcd subdir &&\n-\t\ttest_must_fail git merge master >/dev/null 2>&1 &&\n-\t\t( yes \"\" | git mergetool file3 >/dev/null 2>&1 ) &&\n-\t\t( yes \"\" | git mergetool ../file1 >/dev/null 2>&1 ) &&\n-\t\t( yes \"\" | git mergetool ../file2 ../spaced\\ name >/dev/null 2>&1 ) &&\n-\t\t( yes \"\" | git mergetool ../both >/dev/null 2>&1 ) &&\n-\t\t( yes \"d\" | git mergetool ../file11 >/dev/null 2>&1 ) &&\n-\t\t( yes \"d\" | git mergetool ../file12 >/dev/null 2>&1 ) &&\n-\t\t( yes \"l\" | git mergetool ../submod >/dev/null 2>&1 ) &&\n+\t\ttest_must_fail git merge master &&\n+\t\t( yes \"\" | git mergetool file3 ) &&\n+\t\t( yes \"\" | git mergetool ../file1 ) &&\n+\t\t( yes \"\" | git mergetool ../file2 ../spaced\\ name ) &&\n+\t\t( yes \"\" | git mergetool ../both ) &&\n+\t\t( yes \"d\" | git mergetool ../file11 ) &&\n+\t\t( yes \"d\" | git mergetool ../file12 ) &&\n+\t\t( yes \"l\" | git mergetool ../submod ) &&\n \t\ttest \"$(cat ../file1)\" = \"master updated\" &&\n \t\ttest \"$(cat ../file2)\" = \"master new\" &&\n \t\ttest \"$(cat ../submod/bar)\" = \"branch1 submodule\" &&\n@@ -209,9 +209,9 @@ test_expect_success 'mergetool skips autoresolved' '\n \tgit submodule update -N &&\n \ttest_must_fail git merge master &&\n \ttest -n \"$(git ls-files -u)\" &&\n-\t( yes \"d\" | git mergetool file11 >/dev/null 2>&1 ) &&\n-\t( yes \"d\" | git mergetool file12 >/dev/null 2>&1 ) &&\n-\t( yes \"l\" | git mergetool submod >/dev/null 2>&1 ) &&\n+\t( yes \"d\" | git mergetool file11 ) &&\n+\t( yes \"d\" | git mergetool file12 ) &&\n+\t( yes \"l\" | git mergetool submod ) &&\n \toutput=\"$(git mergetool --no-prompt)\" &&\n \ttest \"$output\" = \"No files need merging\"\n '\n@@ -259,9 +259,9 @@ test_expect_success 'mergetool skips resolved paths when rerere is active' '\n \trm -rf .git/rr-cache &&\n \tgit checkout -b test$test_count branch1 &&\n \tgit submodule update -N &&\n-\ttest_must_fail git merge master >/dev/null 2>&1 &&\n-\t( yes \"l\" | git mergetool --no-prompt submod >/dev/null 2>&1 ) &&\n-\t( yes \"d\" \"d\" | git mergetool --no-prompt >/dev/null 2>&1 ) &&\n+\ttest_must_fail git merge master &&\n+\t( yes \"l\" | git mergetool --no-prompt submod ) &&\n+\t( yes \"d\" \"d\" | git mergetool --no-prompt ) &&\n \tgit submodule update -N &&\n \toutput=\"$(yes \"n\" | git mergetool --no-prompt)\" &&\n \ttest \"$output\" = \"No files need merging\"\n@@ -369,9 +369,9 @@ test_expect_success 'deleted vs modified submodule' '\n \tgit checkout -b test$test_count.a test$test_count &&\n \ttest_must_fail git merge master &&\n \ttest -n \"$(git ls-files -u)\" &&\n-\t( yes \"\" | git mergetool file1 file2 spaced\\ name subdir/file3 >/dev/null 2>&1 ) &&\n-\t( yes \"\" | git mergetool both >/dev/null 2>&1 ) &&\n-\t( yes \"d\" | git mergetool file11 file12 >/dev/null 2>&1 ) &&\n+\t( yes \"\" | git mergetool file1 file2 spaced\\ name subdir/file3 ) &&\n+\t( yes \"\" | git mergetool both ) &&\n+\t( yes \"d\" | git mergetool file11 file12 ) &&\n \t( yes \"r\" | git mergetool submod ) &&\n \trmdir submod && mv submod-movedaside submod &&\n \ttest \"$(cat submod/bar)\" = \"branch1 submodule\" &&\n@@ -386,9 +386,9 @@ test_expect_success 'deleted vs modified submodule' '\n \tgit submodule update -N &&\n \ttest_must_fail git merge master &&\n \ttest -n \"$(git ls-files -u)\" &&\n-\t( yes \"\" | git mergetool file1 file2 spaced\\ name subdir/file3 >/dev/null 2>&1 ) &&\n-\t( yes \"\" | git mergetool both >/dev/null 2>&1 ) &&\n-\t( yes \"d\" | git mergetool file11 file12 >/dev/null 2>&1 ) &&\n+\t( yes \"\" | git mergetool file1 file2 spaced\\ name subdir/file3 ) &&\n+\t( yes \"\" | git mergetool both ) &&\n+\t( yes \"d\" | git mergetool file11 file12 ) &&\n \t( yes \"l\" | git mergetool submod ) &&\n \ttest ! -e submod &&\n \toutput=\"$(git mergetool --no-prompt)\" &&\n@@ -400,9 +400,9 @@ test_expect_success 'deleted vs modified submodule' '\n \tgit submodule update -N &&\n \ttest_must_fail git merge test$test_count &&\n \ttest -n \"$(git ls-files -u)\" &&\n-\t( yes \"\" | git mergetool file1 file2 spaced\\ name subdir/file3 >/dev/null 2>&1 ) &&\n-\t( yes \"\" | git mergetool both >/dev/null 2>&1 ) &&\n-\t( yes \"d\" | git mergetool file11 file12 >/dev/null 2>&1 ) &&\n+\t( yes \"\" | git mergetool file1 file2 spaced\\ name subdir/file3 ) &&\n+\t( yes \"\" | git mergetool both ) &&\n+\t( yes \"d\" | git mergetool file11 file12 ) &&\n \t( yes \"r\" | git mergetool submod ) &&\n \ttest ! -e submod &&\n \ttest -d submod.orig &&\n@@ -416,9 +416,9 @@ test_expect_success 'deleted vs modified submodule' '\n \tgit submodule update -N &&\n \ttest_must_fail git merge test$test_count &&\n \ttest -n \"$(git ls-files -u)\" &&\n-\t( yes \"\" | git mergetool file1 file2 spaced\\ name subdir/file3 >/dev/null 2>&1 ) &&\n-\t( yes \"\" | git mergetool both >/dev/null 2>&1 ) &&\n-\t( yes \"d\" | git mergetool file11 file12 >/dev/null 2>&1 ) &&\n+\t( yes \"\" | git mergetool file1 file2 spaced\\ name subdir/file3 ) &&\n+\t( yes \"\" | git mergetool both ) &&\n+\t( yes \"d\" | git mergetool file11 file12 ) &&\n \t( yes \"l\" | git mergetool submod ) &&\n \ttest \"$(cat submod/bar)\" = \"master submodule\" &&\n \tgit submodule update -N &&\n@@ -440,9 +440,9 @@ test_expect_success 'file vs modified submodule' '\n \tgit checkout -b test$test_count.a branch1 &&\n \ttest_must_fail git merge master &&\n \ttest -n \"$(git ls-files -u)\" &&\n-\t( yes \"\" | git mergetool file1 file2 spaced\\ name subdir/file3 >/dev/null 2>&1 ) &&\n-\t( yes \"\" | git mergetool both >/dev/null 2>&1 ) &&\n-\t( yes \"d\" | git mergetool file11 file12 >/dev/null 2>&1 ) &&\n+\t( yes \"\" | git mergetool file1 file2 spaced\\ name subdir/file3 ) &&\n+\t( yes \"\" | git mergetool both ) &&\n+\t( yes \"d\" | git mergetool file11 file12 ) &&\n \t( yes \"r\" | git mergetool submod ) &&\n \trmdir submod && mv submod-movedaside submod &&\n \ttest \"$(cat submod/bar)\" = \"branch1 submodule\" &&\n@@ -456,9 +456,9 @@ test_expect_success 'file vs modified submodule' '\n \tgit checkout -b test$test_count.b test$test_count &&\n \ttest_must_fail git merge master &&\n \ttest -n \"$(git ls-files -u)\" &&\n-\t( yes \"\" | git mergetool file1 file2 spaced\\ name subdir/file3 >/dev/null 2>&1 ) &&\n-\t( yes \"\" | git mergetool both >/dev/null 2>&1 ) &&\n-\t( yes \"d\" | git mergetool file11 file12 >/dev/null 2>&1 ) &&\n+\t( yes \"\" | git mergetool file1 file2 spaced\\ name subdir/file3 ) &&\n+\t( yes \"\" | git mergetool both ) &&\n+\t( yes \"d\" | git mergetool file11 file12 ) &&\n \t( yes \"l\" | git mergetool submod ) &&\n \tgit submodule update -N &&\n \ttest \"$(cat submod)\" = \"not a submodule\" &&\n@@ -472,9 +472,9 @@ test_expect_success 'file vs modified submodule' '\n \tgit submodule update -N &&\n \ttest_must_fail git merge test$test_count &&\n \ttest -n \"$(git ls-files -u)\" &&\n-\t( yes \"\" | git mergetool file1 file2 spaced\\ name subdir/file3 >/dev/null 2>&1 ) &&\n-\t( yes \"\" | git mergetool both >/dev/null 2>&1 ) &&\n-\t( yes \"d\" | git mergetool file11 file12 >/dev/null 2>&1 ) &&\n+\t( yes \"\" | git mergetool file1 file2 spaced\\ name subdir/file3 ) &&\n+\t( yes \"\" | git mergetool both ) &&\n+\t( yes \"d\" | git mergetool file11 file12 ) &&\n \t( yes \"r\" | git mergetool submod ) &&\n \ttest -d submod.orig &&\n \tgit submodule update -N &&\n@@ -488,9 +488,9 @@ test_expect_success 'file vs modified submodule' '\n \tgit submodule update -N &&\n \ttest_must_fail git merge test$test_count &&\n \ttest -n \"$(git ls-files -u)\" &&\n-\t( yes \"\" | git mergetool file1 file2 spaced\\ name subdir/file3 >/dev/null 2>&1 ) &&\n-\t( yes \"\" | git mergetool both>/dev/null 2>&1 ) &&\n-\t( yes \"d\" | git mergetool file11 file12 >/dev/null 2>&1 ) &&\n+\t( yes \"\" | git mergetool file1 file2 spaced\\ name subdir/file3 ) &&\n+\t( yes \"\" | git mergetool both ) &&\n+\t( yes \"d\" | git mergetool file11 file12 ) &&\n \t( yes \"l\" | git mergetool submod ) &&\n \ttest \"$(cat submod/bar)\" = \"master submodule\" &&\n \tgit submodule update -N &&\n@@ -543,7 +543,7 @@ test_expect_success 'submodule in subdirectory' '\n \tgit add subdir/subdir_module &&\n \tgit commit -m \"change submodule in subdirectory on test$test_count.b\" &&\n \n-\ttest_must_fail git merge test$test_count.a >/dev/null 2>&1 &&\n+\ttest_must_fail git merge test$test_count.a &&\n \t(\n \t\tcd subdir &&\n \t\t( yes \"l\" | git mergetool subdir_module )\n@@ -554,7 +554,7 @@ test_expect_success 'submodule in subdirectory' '\n \tgit reset --hard &&\n \tgit submodule update -N &&\n \n-\ttest_must_fail git merge test$test_count.a >/dev/null 2>&1 &&\n+\ttest_must_fail git merge test$test_count.a &&\n \t( yes \"r\" | git mergetool subdir/subdir_module ) &&\n \ttest \"$(cat subdir/subdir_module/file15)\" = \"test$test_count.b\" &&\n \tgit submodule update -N &&\n@@ -641,7 +641,7 @@ test_expect_success 'filenames seen by tools start with ./' '\n \ttest_config mergetool.myecho.trustExitCode true &&\n \ttest_must_fail git merge master &&\n \tgit mergetool --no-prompt --tool myecho -- both >actual &&\n-\tgrep ^\\./both_LOCAL_ actual >/dev/null\n+\tgrep ^\\./both_LOCAL_ actual\n '\n \n test_lazy_prereq MKTEMP '\n@@ -658,8 +658,8 @@ test_expect_success MKTEMP 'temporary filenames are used with mergetool.writeToT\n \ttest_config mergetool.myecho.trustExitCode true &&\n \ttest_must_fail git merge master &&\n \tgit mergetool --no-prompt --tool myecho -- both >actual &&\n-\t! grep ^\\./both_LOCAL_ actual >/dev/null &&\n-\tgrep /both_LOCAL_ actual >/dev/null\n+\t! grep ^\\./both_LOCAL_ actual &&\n+\tgrep /both_LOCAL_ actual\n '\n \n test_expect_success 'diff.orderFile configuration is honored' '\n-- \n2.21.0.1000.g7817e26e80\n\n"},{"id":"374431","messageId":"0f632ca6bf4f5e76d18b17435a8e565b4cdc3cc0.1556142510.git.liu.denton@gmail.com","threadId":"50966","inReplyTo":"cover.1556142510.git.liu.denton@gmail.com","subject":"[PATCH v3 2/6] t7610: add mergetool --gui tests","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2019-04-24T22:46:59Z","receivedAt":"2019-04-24T22:47:03Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"In 063f2bdbf7 (mergetool: accept -g/--[no-]gui as arguments,\n2018-10-24), mergetool was taught the --gui option but no tests were\nadded to ensure that it was working properly. Add a test to ensure that\nit works.\n\nSigned-off-by: Denton Liu \n---\n t/t7610-mergetool.sh | 22 ++++++++++++++++++++++\n 1 file changed, 22 insertions(+)\n\ndiff --git a/t/t7610-mergetool.sh b/t/t7610-mergetool.sh\nindex 69711487dd..dad607e186 100755\n--- a/t/t7610-mergetool.sh\n+++ b/t/t7610-mergetool.sh\n@@ -145,6 +145,28 @@ test_expect_success 'custom mergetool' '\n \tgit commit -m \"branch1 resolved with mergetool\"\n '\n \n+test_expect_success 'gui mergetool' '\n+\ttest_config merge.guitool myguitool &&\n+\ttest_config mergetool.myguitool.cmd \"(printf \\\"gui \\\" && cat \\\"\\$REMOTE\\\") >\\\"\\$MERGED\\\"\" &&\n+\ttest_config mergetool.myguitool.trustExitCode true &&\n+\ttest_when_finished \"git reset --hard\" &&\n+\tgit checkout -b test$test_count branch1 &&\n+\tgit submodule update -N &&\n+\ttest_must_fail git merge master &&\n+\t( yes \"\" | git mergetool --gui both ) &&\n+\t( yes \"\" | git mergetool -g file1 file1 ) &&\n+\t( yes \"\" | git mergetool --gui file2 \"spaced name\" ) &&\n+\t( yes \"\" | git mergetool --gui subdir/file3 ) &&\n+\t( yes \"d\" | git mergetool --gui file11 ) &&\n+\t( yes \"d\" | git mergetool --gui file12 ) &&\n+\t( yes \"l\" | git mergetool --gui submod ) &&\n+\ttest \"$(cat file1)\" = \"gui master updated\" &&\n+\ttest \"$(cat file2)\" = \"gui master new\" &&\n+\ttest \"$(cat subdir/file3)\" = \"gui master new sub\" &&\n+\ttest \"$(cat submod/bar)\" = \"branch1 submodule\" &&\n+\tgit commit -m \"branch1 resolved with mergetool\"\n+'\n+\n test_expect_success 'mergetool crlf' '\n \ttest_when_finished \"git reset --hard\" &&\n \t# This test_config line must go after the above reset line so that\n-- \n2.21.0.1000.g7817e26e80\n\n"},{"id":"374432","messageId":"ff83d25ff2d0b9585b52c7e28cd7b93292b5304d.1556142510.git.liu.denton@gmail.com","threadId":"50966","inReplyTo":"cover.1556142510.git.liu.denton@gmail.com","subject":"[PATCH v3 3/6] mergetool: use get_merge_tool function","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2019-04-24T22:47:00Z","receivedAt":"2019-04-24T22:47:05Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"In git-mergetool, the logic for getting which merge tool to use is\nduplicated in git-mergetool--lib, except for the fact that it needs to\nknow whether the tool was guessed or not.\n\nRewrite `get_merge_tool` to return whether or not the tool was guessed\nthrough the return code and make git-mergetool call this function\ninstead of duplicating the logic. Note that 1 was chosen to be the\nreturn code of when a tool is guessed because it seems like a slightly\nmore abnormal condition than getting a tool that's explicitly specified\nbut this is completely arbitrary.\n\nAlso, let `$GIT_MERGETOOL_GUI` be set to determine whether or not the\nguitool will be selected.\n\nThis change is not completely backwards compatible as there may be\nexternal users of git-mergetool--lib. However, only one user,\ngit-diffall[1], was found from searching GitHub and Google. It seems\nvery unlikely that there exists an external caller that would take into\naccount the return code of `get_merge_tool` as it would always return 0\nbefore this change so this change probably does not affect any external\nusers.\n\n[1]: https://github.com/thenigan/git-diffall\n\nSigned-off-by: Denton Liu \n---\n Documentation/git-mergetool--lib.txt | 4 +++-\n git-difftool--helper.sh | 2 +-\n git-mergetool--lib.sh | 5 ++++-\n git-mergetool.sh | 12 ++++--------\n 4 files changed, 12 insertions(+), 11 deletions(-)\n\ndiff --git a/Documentation/git-mergetool--lib.txt b/Documentation/git-mergetool--lib.txt\nindex 055550b2bc..4da9d24096 100644\n--- a/Documentation/git-mergetool--lib.txt\n+++ b/Documentation/git-mergetool--lib.txt\n@@ -28,7 +28,9 @@ to define the operation mode for the functions listed below.\n FUNCTIONS\n ---------\n get_merge_tool::\n-\treturns a merge tool.\n+\treturns a merge tool. the return code is 1 if we returned a guessed\n+\tmerge tool, else 0. '$GIT_MERGETOOL_GUI' may be set to 'true' to\n+\tsearch for the appropriate guitool.\n \n get_merge_tool_cmd::\n \treturns the custom command for a merge tool.\ndiff --git a/git-difftool--helper.sh b/git-difftool--helper.sh\nindex 7bfb6737df..46af3e60b7 100755\n--- a/git-difftool--helper.sh\n+++ b/git-difftool--helper.sh\n@@ -71,7 +71,7 @@ then\n \tthen\n \t\tmerge_tool=\"$GIT_DIFF_TOOL\"\n \telse\n-\t\tmerge_tool=\"$(get_merge_tool)\" || exit\n+\t\tmerge_tool=\"$(get_merge_tool)\"\n \tfi\n fi\n \ndiff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh\nindex 83bf52494c..68ff26a0f7 100644\n--- a/git-mergetool--lib.sh\n+++ b/git-mergetool--lib.sh\n@@ -403,14 +403,17 @@ get_merge_tool_path () {\n }\n \n get_merge_tool () {\n+\tnot_guessed=true\n \t# Check if a merge tool has been configured\n-\tmerge_tool=$(get_configured_merge_tool)\n+\tmerge_tool=$(get_configured_merge_tool $GIT_MERGETOOL_GUI)\n \t# Try to guess an appropriate merge tool if no tool has been set.\n \tif test -z \"$merge_tool\"\n \tthen\n \t\tmerge_tool=$(guess_merge_tool) || exit\n+\t\tnot_guessed=false\n \tfi\n \techo \"$merge_tool\"\n+\ttest \"$not_guessed\" = true\n }\n \n mergetool_find_win32_cmd () {\ndiff --git a/git-mergetool.sh b/git-mergetool.sh\nindex 01b9ad59b2..88fa6a914a 100755\n--- a/git-mergetool.sh\n+++ b/git-mergetool.sh\n@@ -389,7 +389,7 @@ print_noop_and_exit () {\n \n main () {\n \tprompt=$(git config --bool mergetool.prompt)\n-\tgui_tool=false\n+\tGIT_MERGETOOL_GUI=false\n \tguessed_merge_tool=false\n \torderfile=\n \n@@ -416,10 +416,10 @@ main () {\n \t\t\tesac\n \t\t\t;;\n \t\t--no-gui)\n-\t\t\tgui_tool=false\n+\t\t\tGIT_MERGETOOL_GUI=false\n \t\t\t;;\n \t\t-g|--gui)\n-\t\t\tgui_tool=true\n+\t\t\tGIT_MERGETOOL_GUI=true\n \t\t\t;;\n \t\t-y|--no-prompt)\n \t\t\tprompt=false\n@@ -449,12 +449,8 @@ main () {\n \n \tif test -z \"$merge_tool\"\n \tthen\n-\t\t# Check if a merge tool has been configured\n-\t\tmerge_tool=$(get_configured_merge_tool $gui_tool)\n-\t\t# Try to guess an appropriate merge tool if no tool has been set.\n-\t\tif test -z \"$merge_tool\"\n+\t\tif ! merge_tool=$(get_merge_tool)\n \t\tthen\n-\t\t\tmerge_tool=$(guess_merge_tool) || exit\n \t\t\tguessed_merge_tool=true\n \t\tfi\n \tfi\n-- \n2.21.0.1000.g7817e26e80\n\n"},{"id":"374433","messageId":"e975fe4a8b206d8e40f9c4d7cd278fdb5c7358f0.1556142510.git.liu.denton@gmail.com","threadId":"50966","inReplyTo":"cover.1556142510.git.liu.denton@gmail.com","subject":"[PATCH v3 4/6] mergetool: fallback to tool when guitool unavailable","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2019-04-24T22:47:01Z","receivedAt":"2019-04-24T22:47:06Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"In git-difftool, if the tool is called with --gui but `diff.guitool` is\nnot set, it falls back to `diff.tool`. Make git-mergetool also fallback\nfrom `merge.guitool` to `merge.tool` if the former is undefined.\n\nIf git-difftool were to use `get_configured_mergetool`, it would also\nget the fallback behaviour in the following precedence:\n\n1. diff.guitool\n2. merge.guitool\n3. diff.tool\n4. merge.tool\n\nSigned-off-by: Denton Liu \n---\n Documentation/git-mergetool.txt | 4 +++-\n git-mergetool--lib.sh | 27 ++++++++++++++++++---------\n t/t7610-mergetool.sh | 19 +++++++++++++++++++\n 3 files changed, 40 insertions(+), 10 deletions(-)\n\ndiff --git a/Documentation/git-mergetool.txt b/Documentation/git-mergetool.txt\nindex 0c7975a050..6b14702e78 100644\n--- a/Documentation/git-mergetool.txt\n+++ b/Documentation/git-mergetool.txt\n@@ -83,7 +83,9 @@ success of the resolution after the custom tool has exited.\n --gui::\n \tWhen 'git-mergetool' is invoked with the `-g` or `--gui` option\n \tthe default merge tool will be read from the configured\n-\t`merge.guitool` variable instead of `merge.tool`.\n+\t`merge.guitool` variable instead of `merge.tool`. If\n+\t`merge.guitool` is not set, we will fallback to the tool\n+\tconfigured under `merge.tool`.\n \n --no-gui::\n \tThis overrides a previous `-g` or `--gui` setting and reads the\ndiff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh\nindex 68ff26a0f7..ada16acffc 100644\n--- a/git-mergetool--lib.sh\n+++ b/git-mergetool--lib.sh\n@@ -350,20 +350,29 @@ guess_merge_tool () {\n }\n \n get_configured_merge_tool () {\n-\t# If first argument is true, find the guitool instead\n-\tif test \"$1\" = true\n+\tis_gui=\"$1\"\n+\tsections=\"merge\"\n+\tkeys=\"tool\"\n+\n+\tif diff_mode\n \tthen\n-\t\tgui_prefix=gui\n+\t\tsections=\"diff $sections\"\n \tfi\n \n-\t# Diff mode first tries diff.(gui)tool and falls back to merge.(gui)tool.\n-\t# Merge mode only checks merge.(gui)tool\n-\tif diff_mode\n+\tif \"$is_gui\" = true\n \tthen\n-\t\tmerge_tool=$(git config diff.${gui_prefix}tool || git config merge.${gui_prefix}tool)\n-\telse\n-\t\tmerge_tool=$(git config merge.${gui_prefix}tool)\n+\t\tkeys=\"guitool $keys\"\n \tfi\n+\n+\tIFS=' '\n+\tfor key in $keys\n+\tdo\n+\t\tfor section in $sections\n+\t\tdo\n+\t\t\tmerge_tool=$(git config $section.$key) && break 2\n+\t\tdone\n+\tdone\n+\n \tif test -n \"$merge_tool\" && ! valid_tool \"$merge_tool\"\n \tthen\n \t\techo >&2 \"git config option $TOOL_MODE.${gui_prefix}tool set to unknown tool: $merge_tool\"\ndiff --git a/t/t7610-mergetool.sh b/t/t7610-mergetool.sh\nindex dad607e186..5b61c10a9c 100755\n--- a/t/t7610-mergetool.sh\n+++ b/t/t7610-mergetool.sh\n@@ -167,6 +167,25 @@ test_expect_success 'gui mergetool' '\n \tgit commit -m \"branch1 resolved with mergetool\"\n '\n \n+test_expect_success 'gui mergetool without merge.guitool set falls back to merge.tool' '\n+\ttest_when_finished \"git reset --hard\" &&\n+\tgit checkout -b test$test_count branch1 &&\n+\tgit submodule update -N &&\n+\ttest_must_fail git merge master &&\n+\t( yes \"\" | git mergetool --gui both ) &&\n+\t( yes \"\" | git mergetool -g file1 file1 ) &&\n+\t( yes \"\" | git mergetool --gui file2 \"spaced name\" ) &&\n+\t( yes \"\" | git mergetool --gui subdir/file3 ) &&\n+\t( yes \"d\" | git mergetool --gui file11 ) &&\n+\t( yes \"d\" | git mergetool --gui file12 ) &&\n+\t( yes \"l\" | git mergetool --gui submod ) &&\n+\ttest \"$(cat file1)\" = \"master updated\" &&\n+\ttest \"$(cat file2)\" = \"master new\" &&\n+\ttest \"$(cat subdir/file3)\" = \"master new sub\" &&\n+\ttest \"$(cat submod/bar)\" = \"branch1 submodule\" &&\n+\tgit commit -m \"branch1 resolved with mergetool\"\n+'\n+\n test_expect_success 'mergetool crlf' '\n \ttest_when_finished \"git reset --hard\" &&\n \t# This test_config line must go after the above reset line so that\n-- \n2.21.0.1000.g7817e26e80\n\n"},{"id":"374434","messageId":"bc3e2291714d48d0fc8c420c7cb54219c251887d.1556142510.git.liu.denton@gmail.com","threadId":"50966","inReplyTo":"cover.1556142510.git.liu.denton@gmail.com","subject":"[PATCH v3 5/6] difftool: make --gui, --tool and --extcmd mutually exclusive","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2019-04-24T22:47:03Z","receivedAt":"2019-04-24T22:47:08Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"In git-difftool, these options specify which tool to ultimately run. As\na result, they are logically conflicting. Explicitly disallow these\noptions from being used together.\n\nSigned-off-by: Denton Liu \n---\n builtin/difftool.c | 3 +++\n t/t7800-difftool.sh | 8 ++++++++\n 2 files changed, 11 insertions(+)\n\ndiff --git a/builtin/difftool.c b/builtin/difftool.c\nindex a3ea60ea71..65bba90338 100644\n--- a/builtin/difftool.c\n+++ b/builtin/difftool.c\n@@ -731,6 +731,9 @@ int cmd_difftool(int argc, const char **argv, const char *prefix)\n \tsetenv(GIT_DIR_ENVIRONMENT, absolute_path(get_git_dir()), 1);\n \tsetenv(GIT_WORK_TREE_ENVIRONMENT, absolute_path(get_git_work_tree()), 1);\n \n+\tif (use_gui_tool + !!difftool_cmd + !!extcmd > 1)\n+\t\tdie(_(\"--gui, --tool and --extcmd are mutually exclusive\"));\n+\n \tif (use_gui_tool && diff_gui_tool && *diff_gui_tool)\n \t\tsetenv(\"GIT_DIFF_TOOL\", diff_gui_tool, 1);\n \telse if (difftool_cmd) {\ndiff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh\nindex bb9a7f4ff9..107f31213d 100755\n--- a/t/t7800-difftool.sh\n+++ b/t/t7800-difftool.sh\n@@ -705,4 +705,12 @@ test_expect_success SYMLINKS 'difftool --dir-diff handles modified symlinks' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'difftool --gui, --tool and --extcmd are exclusive' '\n+\tdifftool_test_setup &&\n+\ttest_must_fail git difftool --gui --tool=test-tool &&\n+\ttest_must_fail git difftool --gui --extcmd=cat &&\n+\ttest_must_fail git difftool --tool=test-tool --extcmd=cat &&\n+\ttest_must_fail git difftool --gui --tool=test-tool --extcmd=cat\n+'\n+\n test_done\n-- \n2.21.0.1000.g7817e26e80\n\n"},{"id":"374435","messageId":"f39b15efbd5c765953737f99aa032230f066c074.1556142510.git.liu.denton@gmail.com","threadId":"50966","inReplyTo":"cover.1556142510.git.liu.denton@gmail.com","subject":"[PATCH v3 6/6] difftool: fallback on merge.guitool","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2019-04-24T22:47:04Z","receivedAt":"2019-04-24T22:47:09Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"In git-difftool.txt, it says\n\n\t'git difftool' falls back to 'git mergetool' config variables when the\n\tdifftool equivalents have not been defined.\n\nHowever, when `diff.guitool` is missing, it doesn't fallback to\nanything. Make git-difftool fallback to `merge.guitool` when `diff.guitool` is\nmissing.\n\nSigned-off-by: Denton Liu \n---\n Documentation/git-difftool.txt | 4 +++-\n builtin/difftool.c | 10 ++--------\n t/t7800-difftool.sh | 16 ++++++++++++++++\n 3 files changed, 21 insertions(+), 9 deletions(-)\n\ndiff --git a/Documentation/git-difftool.txt b/Documentation/git-difftool.txt\nindex 96c26e6aa8..484c485fd0 100644\n--- a/Documentation/git-difftool.txt\n+++ b/Documentation/git-difftool.txt\n@@ -90,7 +90,9 @@ instead. `--no-symlinks` is the default on Windows.\n \tWhen 'git-difftool' is invoked with the `-g` or `--gui` option\n \tthe default diff tool will be read from the configured\n \t`diff.guitool` variable instead of `diff.tool`. The `--no-gui`\n-\toption can be used to override this setting.\n+\toption can be used to override this setting. If `diff.guitool`\n+\tis not set, we will fallback in the order of `merge.guitool`,\n+\t`diff.tool`, `merge.tool` until a tool is found.\n \n --[no-]trust-exit-code::\n \t'git-difftool' invokes a diff tool individually on each file.\ndiff --git a/builtin/difftool.c b/builtin/difftool.c\nindex 65bba90338..10660639c0 100644\n--- a/builtin/difftool.c\n+++ b/builtin/difftool.c\n@@ -24,7 +24,6 @@\n #include \"object-store.h\"\n #include \"dir.h\"\n \n-static char *diff_gui_tool;\n static int trust_exit_code;\n \n static const char *const builtin_difftool_usage[] = {\n@@ -34,11 +33,6 @@ static const char *const builtin_difftool_usage[] = {\n \n static int difftool_config(const char *var, const char *value, void *cb)\n {\n-\tif (!strcmp(var, \"diff.guitool\")) {\n-\t\tdiff_gui_tool = xstrdup(value);\n-\t\treturn 0;\n-\t}\n-\n \tif (!strcmp(var, \"difftool.trustexitcode\")) {\n \t\ttrust_exit_code = git_config_bool(var, value);\n \t\treturn 0;\n@@ -734,8 +728,8 @@ int cmd_difftool(int argc, const char **argv, const char *prefix)\n \tif (use_gui_tool + !!difftool_cmd + !!extcmd > 1)\n \t\tdie(_(\"--gui, --tool and --extcmd are mutually exclusive\"));\n \n-\tif (use_gui_tool && diff_gui_tool && *diff_gui_tool)\n-\t\tsetenv(\"GIT_DIFF_TOOL\", diff_gui_tool, 1);\n+\tif (use_gui_tool)\n+\t\tsetenv(\"GIT_MERGETOOL_GUI\", \"true\", 1);\n \telse if (difftool_cmd) {\n \t\tif (*difftool_cmd)\n \t\t\tsetenv(\"GIT_DIFF_TOOL\", difftool_cmd, 1);\ndiff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh\nindex 107f31213d..ae90701a12 100755\n--- a/t/t7800-difftool.sh\n+++ b/t/t7800-difftool.sh\n@@ -279,11 +279,27 @@ test_expect_success 'difftool + mergetool config variables' '\n \techo branch >expect &&\n \tgit difftool --no-prompt branch >actual &&\n \ttest_cmp expect actual &&\n+\tgit difftool --gui --no-prompt branch >actual &&\n+\ttest_cmp expect actual &&\n \n \t# set merge.tool to something bogus, diff.tool to test-tool\n \ttest_config merge.tool bogus-tool &&\n \ttest_config diff.tool test-tool &&\n \tgit difftool --no-prompt branch >actual &&\n+\ttest_cmp expect actual &&\n+\tgit difftool --gui --no-prompt branch >actual &&\n+\ttest_cmp expect actual &&\n+\n+\t# set merge.tool, diff.tool to something bogus, merge.guitool to test-tool\n+\ttest_config diff.tool bogus-tool &&\n+\ttest_config merge.guitool test-tool &&\n+\tgit difftool --gui --no-prompt branch >actual &&\n+\ttest_cmp expect actual &&\n+\n+\t# set merge.tool, diff.tool, merge.guitool to something bogus, diff.guitool to test-tool\n+\ttest_config merge.guitool bogus-tool &&\n+\ttest_config diff.guitool test-tool &&\n+\tgit difftool --gui --no-prompt branch >actual &&\n \ttest_cmp expect actual\n '\n \n-- \n2.21.0.1000.g7817e26e80\n\n"},{"id":"374447","messageId":"xmqqftq6zugi.fsf@gitster-ct.c.googlers.com","threadId":"50966","inReplyTo":"9f9922cab341396a846edc2f52342c59d294379a.1556142510.git.liu.denton@gmail.com","subject":"Re: [PATCH v3 1/6] t7610: unsuppress output","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-04-25T02:31:41Z","receivedAt":"2019-04-25T02:36:09Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Denton Liu writes:\n\n> The output for commands used to be suppressed by redirecting both stdout\n> and stderr to /dev/null. However, this should not happen since the\n> output is useful for debugging and, without the \"-v\" flag, test scripts\n> don't output anyway.\n>\n> Unsuppress the output by removing the redirections to /dev/null.\n\nThanks for clearly justifying the clean-up.\n\nI take it that with this there is no behaviour changes (e.g. a\ncommand that may change its behaviour depending on where its\nstandard output goes does not change the outcome of the tests)?\n\nI am not worried about the invocations of \"git mergetool\" downstream\nof the pipe that is fed by \"yes\", and the remainder are a handful of\n\"git merge\" invocations, which looked OK to me.\n\nThanks.\n\n> Signed-off-by: Denton Liu \n> ---\n> t/t7610-mergetool.sh | 122 +++++++++++++++++++++----------------------\n> 1 file changed, 61 insertions(+), 61 deletions(-)\n>\n> diff --git a/t/t7610-mergetool.sh b/t/t7610-mergetool.sh\n> index a9fb971615..69711487dd 100755\n> --- a/t/t7610-mergetool.sh\n> +++ b/t/t7610-mergetool.sh\n> @@ -130,14 +130,14 @@ test_expect_success 'custom mergetool' '\n> \ttest_when_finished \"git reset --hard\" &&\n> \tgit checkout -b test$test_count branch1 &&\n> \tgit submodule update -N &&\n> -\ttest_must_fail git merge master >/dev/null 2>&1 &&\n> -\t( yes \"\" | git mergetool both >/dev/null 2>&1 ) &&\n> +\ttest_must_fail git merge master &&\n> +\t( yes \"\" | git mergetool both ) &&\n> \t( yes \"\" | git mergetool file1 file1 ) &&\n> -\t( yes \"\" | git mergetool file2 \"spaced name\" >/dev/null 2>&1 ) &&\n> -\t( yes \"\" | git mergetool subdir/file3 >/dev/null 2>&1 ) &&\n> -\t( yes \"d\" | git mergetool file11 >/dev/null 2>&1 ) &&\n> -\t( yes \"d\" | git mergetool file12 >/dev/null 2>&1 ) &&\n> -\t( yes \"l\" | git mergetool submod >/dev/null 2>&1 ) &&\n> +\t( yes \"\" | git mergetool file2 \"spaced name\" ) &&\n> +\t( yes \"\" | git mergetool subdir/file3 ) &&\n> +\t( yes \"d\" | git mergetool file11 ) &&\n> +\t( yes \"d\" | git mergetool file12 ) &&\n> +\t( yes \"l\" | git mergetool submod ) &&\n> \ttest \"$(cat file1)\" = \"master updated\" &&\n> \ttest \"$(cat file2)\" = \"master new\" &&\n> \ttest \"$(cat subdir/file3)\" = \"master new sub\" &&\n> @@ -153,15 +153,15 @@ test_expect_success 'mergetool crlf' '\n> \t# test_when_finished is LIFO.)\n> \ttest_config core.autocrlf true &&\n> \tgit checkout -b test$test_count branch1 &&\n> -\ttest_must_fail git merge master >/dev/null 2>&1 &&\n> -\t( yes \"\" | git mergetool file1 >/dev/null 2>&1 ) &&\n> -\t( yes \"\" | git mergetool file2 >/dev/null 2>&1 ) &&\n> -\t( yes \"\" | git mergetool \"spaced name\" >/dev/null 2>&1 ) &&\n> -\t( yes \"\" | git mergetool both >/dev/null 2>&1 ) &&\n> -\t( yes \"\" | git mergetool subdir/file3 >/dev/null 2>&1 ) &&\n> -\t( yes \"d\" | git mergetool file11 >/dev/null 2>&1 ) &&\n> -\t( yes \"d\" | git mergetool file12 >/dev/null 2>&1 ) &&\n> -\t( yes \"r\" | git mergetool submod >/dev/null 2>&1 ) &&\n> +\ttest_must_fail git merge master &&\n> +\t( yes \"\" | git mergetool file1 ) &&\n> +\t( yes \"\" | git mergetool file2 ) &&\n> +\t( yes \"\" | git mergetool \"spaced name\" ) &&\n> +\t( yes \"\" | git mergetool both ) &&\n> +\t( yes \"\" | git mergetool subdir/file3 ) &&\n> +\t( yes \"d\" | git mergetool file11 ) &&\n> +\t( yes \"d\" | git mergetool file12 ) &&\n> +\t( yes \"r\" | git mergetool submod ) &&\n> \ttest \"$(printf x | cat file1 -)\" = \"$(printf \"master updated\\r\\nx\")\" &&\n> \ttest \"$(printf x | cat file2 -)\" = \"$(printf \"master new\\r\\nx\")\" &&\n> \ttest \"$(printf x | cat subdir/file3 -)\" = \"$(printf \"master new sub\\r\\nx\")\" &&\n> @@ -176,8 +176,8 @@ test_expect_success 'mergetool in subdir' '\n> \tgit submodule update -N &&\n> \t(\n> \t\tcd subdir &&\n> -\t\ttest_must_fail git merge master >/dev/null 2>&1 &&\n> -\t\t( yes \"\" | git mergetool file3 >/dev/null 2>&1 ) &&\n> +\t\ttest_must_fail git merge master &&\n> +\t\t( yes \"\" | git mergetool file3 ) &&\n> \t\ttest \"$(cat file3)\" = \"master new sub\"\n> \t)\n> '\n> @@ -188,14 +188,14 @@ test_expect_success 'mergetool on file in parent dir' '\n> \tgit submodule update -N &&\n> \t(\n> \t\tcd subdir &&\n> -\t\ttest_must_fail git merge master >/dev/null 2>&1 &&\n> -\t\t( yes \"\" | git mergetool file3 >/dev/null 2>&1 ) &&\n> -\t\t( yes \"\" | git mergetool ../file1 >/dev/null 2>&1 ) &&\n> -\t\t( yes \"\" | git mergetool ../file2 ../spaced\\ name >/dev/null 2>&1 ) &&\n> -\t\t( yes \"\" | git mergetool ../both >/dev/null 2>&1 ) &&\n> -\t\t( yes \"d\" | git mergetool ../file11 >/dev/null 2>&1 ) &&\n> -\t\t( yes \"d\" | git mergetool ../file12 >/dev/null 2>&1 ) &&\n> -\t\t( yes \"l\" | git mergetool ../submod >/dev/null 2>&1 ) &&\n> +\t\ttest_must_fail git merge master &&\n> +\t\t( yes \"\" | git mergetool file3 ) &&\n> +\t\t( yes \"\" | git mergetool ../file1 ) &&\n> +\t\t( yes \"\" | git mergetool ../file2 ../spaced\\ name ) &&\n> +\t\t( yes \"\" | git mergetool ../both ) &&\n> +\t\t( yes \"d\" | git mergetool ../file11 ) &&\n> +\t\t( yes \"d\" | git mergetool ../file12 ) &&\n> +\t\t( yes \"l\" | git mergetool ../submod ) &&\n> \t\ttest \"$(cat ../file1)\" = \"master updated\" &&\n> \t\ttest \"$(cat ../file2)\" = \"master new\" &&\n> \t\ttest \"$(cat ../submod/bar)\" = \"branch1 submodule\" &&\n> @@ -209,9 +209,9 @@ test_expect_success 'mergetool skips autoresolved' '\n> \tgit submodule update -N &&\n> \ttest_must_fail git merge master &&\n> \ttest -n \"$(git ls-files -u)\" &&\n> -\t( yes \"d\" | git mergetool file11 >/dev/null 2>&1 ) &&\n> -\t( yes \"d\" | git mergetool file12 >/dev/null 2>&1 ) &&\n> -\t( yes \"l\" | git mergetool submod >/dev/null 2>&1 ) &&\n> +\t( yes \"d\" | git mergetool file11 ) &&\n> +\t( yes \"d\" | git mergetool file12 ) &&\n> +\t( yes \"l\" | git mergetool submod ) &&\n> \toutput=\"$(git mergetool --no-prompt)\" &&\n> \ttest \"$output\" = \"No files need merging\"\n> '\n> @@ -259,9 +259,9 @@ test_expect_success 'mergetool skips resolved paths when rerere is active' '\n> \trm -rf .git/rr-cache &&\n> \tgit checkout -b test$test_count branch1 &&\n> \tgit submodule update -N &&\n> -\ttest_must_fail git merge master >/dev/null 2>&1 &&\n> -\t( yes \"l\" | git mergetool --no-prompt submod >/dev/null 2>&1 ) &&\n> -\t( yes \"d\" \"d\" | git mergetool --no-prompt >/dev/null 2>&1 ) &&\n> +\ttest_must_fail git merge master &&\n> +\t( yes \"l\" | git mergetool --no-prompt submod ) &&\n> +\t( yes \"d\" \"d\" | git mergetool --no-prompt ) &&\n> \tgit submodule update -N &&\n> \toutput=\"$(yes \"n\" | git mergetool --no-prompt)\" &&\n> \ttest \"$output\" = \"No files need merging\"\n> @@ -369,9 +369,9 @@ test_expect_success 'deleted vs modified submodule' '\n> \tgit checkout -b test$test_count.a test$test_count &&\n> \ttest_must_fail git merge master &&\n> \ttest -n \"$(git ls-files -u)\" &&\n> -\t( yes \"\" | git mergetool file1 file2 spaced\\ name subdir/file3 >/dev/null 2>&1 ) &&\n> -\t( yes \"\" | git mergetool both >/dev/null 2>&1 ) &&\n> -\t( yes \"d\" | git mergetool file11 file12 >/dev/null 2>&1 ) &&\n> +\t( yes \"\" | git mergetool file1 file2 spaced\\ name subdir/file3 ) &&\n> +\t( yes \"\" | git mergetool both ) &&\n> +\t( yes \"d\" | git mergetool file11 file12 ) &&\n> \t( yes \"r\" | git mergetool submod ) &&\n> \trmdir submod && mv submod-movedaside submod &&\n> \ttest \"$(cat submod/bar)\" = \"branch1 submodule\" &&\n> @@ -386,9 +386,9 @@ test_expect_success 'deleted vs modified submodule' '\n> \tgit submodule update -N &&\n> \ttest_must_fail git merge master &&\n> \ttest -n \"$(git ls-files -u)\" &&\n> -\t( yes \"\" | git mergetool file1 file2 spaced\\ name subdir/file3 >/dev/null 2>&1 ) &&\n> -\t( yes \"\" | git mergetool both >/dev/null 2>&1 ) &&\n> -\t( yes \"d\" | git mergetool file11 file12 >/dev/null 2>&1 ) &&\n> +\t( yes \"\" | git mergetool file1 file2 spaced\\ name subdir/file3 ) &&\n> +\t( yes \"\" | git mergetool both ) &&\n> +\t( yes \"d\" | git mergetool file11 file12 ) &&\n> \t( yes \"l\" | git mergetool submod ) &&\n> \ttest ! -e submod &&\n> \toutput=\"$(git mergetool --no-prompt)\" &&\n> @@ -400,9 +400,9 @@ test_expect_success 'deleted vs modified submodule' '\n> \tgit submodule update -N &&\n> \ttest_must_fail git merge test$test_count &&\n> \ttest -n \"$(git ls-files -u)\" &&\n> -\t( yes \"\" | git mergetool file1 file2 spaced\\ name subdir/file3 >/dev/null 2>&1 ) &&\n> -\t( yes \"\" | git mergetool both >/dev/null 2>&1 ) &&\n> -\t( yes \"d\" | git mergetool file11 file12 >/dev/null 2>&1 ) &&\n> +\t( yes \"\" | git mergetool file1 file2 spaced\\ name subdir/file3 ) &&\n> +\t( yes \"\" | git mergetool both ) &&\n> +\t( yes \"d\" | git mergetool file11 file12 ) &&\n> \t( yes \"r\" | git mergetool submod ) &&\n> \ttest ! -e submod &&\n> \ttest -d submod.orig &&\n> @@ -416,9 +416,9 @@ test_expect_success 'deleted vs modified submodule' '\n> \tgit submodule update -N &&\n> \ttest_must_fail git merge test$test_count &&\n> \ttest -n \"$(git ls-files -u)\" &&\n> -\t( yes \"\" | git mergetool file1 file2 spaced\\ name subdir/file3 >/dev/null 2>&1 ) &&\n> -\t( yes \"\" | git mergetool both >/dev/null 2>&1 ) &&\n> -\t( yes \"d\" | git mergetool file11 file12 >/dev/null 2>&1 ) &&\n> +\t( yes \"\" | git mergetool file1 file2 spaced\\ name subdir/file3 ) &&\n> +\t( yes \"\" | git mergetool both ) &&\n> +\t( yes \"d\" | git mergetool file11 file12 ) &&\n> \t( yes \"l\" | git mergetool submod ) &&\n> \ttest \"$(cat submod/bar)\" = \"master submodule\" &&\n> \tgit submodule update -N &&\n> @@ -440,9 +440,9 @@ test_expect_success 'file vs modified submodule' '\n> \tgit checkout -b test$test_count.a branch1 &&\n> \ttest_must_fail git merge master &&\n> \ttest -n \"$(git ls-files -u)\" &&\n> -\t( yes \"\" | git mergetool file1 file2 spaced\\ name subdir/file3 >/dev/null 2>&1 ) &&\n> -\t( yes \"\" | git mergetool both >/dev/null 2>&1 ) &&\n> -\t( yes \"d\" | git mergetool file11 file12 >/dev/null 2>&1 ) &&\n> +\t( yes \"\" | git mergetool file1 file2 spaced\\ name subdir/file3 ) &&\n> +\t( yes \"\" | git mergetool both ) &&\n> +\t( yes \"d\" | git mergetool file11 file12 ) &&\n> \t( yes \"r\" | git mergetool submod ) &&\n> \trmdir submod && mv submod-movedaside submod &&\n> \ttest \"$(cat submod/bar)\" = \"branch1 submodule\" &&\n> @@ -456,9 +456,9 @@ test_expect_success 'file vs modified submodule' '\n> \tgit checkout -b test$test_count.b test$test_count &&\n> \ttest_must_fail git merge master &&\n> \ttest -n \"$(git ls-files -u)\" &&\n> -\t( yes \"\" | git mergetool file1 file2 spaced\\ name subdir/file3 >/dev/null 2>&1 ) &&\n> -\t( yes \"\" | git mergetool both >/dev/null 2>&1 ) &&\n> -\t( yes \"d\" | git mergetool file11 file12 >/dev/null 2>&1 ) &&\n> +\t( yes \"\" | git mergetool file1 file2 spaced\\ name subdir/file3 ) &&\n> +\t( yes \"\" | git mergetool both ) &&\n> +\t( yes \"d\" | git mergetool file11 file12 ) &&\n> \t( yes \"l\" | git mergetool submod ) &&\n> \tgit submodule update -N &&\n> \ttest \"$(cat submod)\" = \"not a submodule\" &&\n> @@ -472,9 +472,9 @@ test_expect_success 'file vs modified submodule' '\n> \tgit submodule update -N &&\n> \ttest_must_fail git merge test$test_count &&\n> \ttest -n \"$(git ls-files -u)\" &&\n> -\t( yes \"\" | git mergetool file1 file2 spaced\\ name subdir/file3 >/dev/null 2>&1 ) &&\n> -\t( yes \"\" | git mergetool both >/dev/null 2>&1 ) &&\n> -\t( yes \"d\" | git mergetool file11 file12 >/dev/null 2>&1 ) &&\n> +\t( yes \"\" | git mergetool file1 file2 spaced\\ name subdir/file3 ) &&\n> +\t( yes \"\" | git mergetool both ) &&\n> +\t( yes \"d\" | git mergetool file11 file12 ) &&\n> \t( yes \"r\" | git mergetool submod ) &&\n> \ttest -d submod.orig &&\n> \tgit submodule update -N &&\n> @@ -488,9 +488,9 @@ test_expect_success 'file vs modified submodule' '\n> \tgit submodule update -N &&\n> \ttest_must_fail git merge test$test_count &&\n> \ttest -n \"$(git ls-files -u)\" &&\n> -\t( yes \"\" | git mergetool file1 file2 spaced\\ name subdir/file3 >/dev/null 2>&1 ) &&\n> -\t( yes \"\" | git mergetool both>/dev/null 2>&1 ) &&\n> -\t( yes \"d\" | git mergetool file11 file12 >/dev/null 2>&1 ) &&\n> +\t( yes \"\" | git mergetool file1 file2 spaced\\ name subdir/file3 ) &&\n> +\t( yes \"\" | git mergetool both ) &&\n> +\t( yes \"d\" | git mergetool file11 file12 ) &&\n> \t( yes \"l\" | git mergetool submod ) &&\n> \ttest \"$(cat submod/bar)\" = \"master submodule\" &&\n> \tgit submodule update -N &&\n> @@ -543,7 +543,7 @@ test_expect_success 'submodule in subdirectory' '\n> \tgit add subdir/subdir_module &&\n> \tgit commit -m \"change submodule in subdirectory on test$test_count.b\" &&\n> \n> -\ttest_must_fail git merge test$test_count.a >/dev/null 2>&1 &&\n> +\ttest_must_fail git merge test$test_count.a &&\n> \t(\n> \t\tcd subdir &&\n> \t\t( yes \"l\" | git mergetool subdir_module )\n> @@ -554,7 +554,7 @@ test_expect_success 'submodule in subdirectory' '\n> \tgit reset --hard &&\n> \tgit submodule update -N &&\n> \n> -\ttest_must_fail git merge test$test_count.a >/dev/null 2>&1 &&\n> +\ttest_must_fail git merge test$test_count.a &&\n> \t( yes \"r\" | git mergetool subdir/subdir_module ) &&\n> \ttest \"$(cat subdir/subdir_module/file15)\" = \"test$test_count.b\" &&\n> \tgit submodule update -N &&\n> @@ -641,7 +641,7 @@ test_expect_success 'filenames seen by tools start with ./' '\n> \ttest_config mergetool.myecho.trustExitCode true &&\n> \ttest_must_fail git merge master &&\n> \tgit mergetool --no-prompt --tool myecho -- both >actual &&\n> -\tgrep ^\\./both_LOCAL_ actual >/dev/null\n> +\tgrep ^\\./both_LOCAL_ actual\n> '\n> \n> test_lazy_prereq MKTEMP '\n> @@ -658,8 +658,8 @@ test_expect_success MKTEMP 'temporary filenames are used with mergetool.writeToT\n> \ttest_config mergetool.myecho.trustExitCode true &&\n> \ttest_must_fail git merge master &&\n> \tgit mergetool --no-prompt --tool myecho -- both >actual &&\n> -\t! grep ^\\./both_LOCAL_ actual >/dev/null &&\n> -\tgrep /both_LOCAL_ actual >/dev/null\n> +\t! grep ^\\./both_LOCAL_ actual &&\n> +\tgrep /both_LOCAL_ actual\n> '\n> \n> test_expect_success 'diff.orderFile configuration is honored' '\n"},{"id":"374448","messageId":"xmqqbm0uzu9a.fsf@gitster-ct.c.googlers.com","threadId":"50966","inReplyTo":"ff83d25ff2d0b9585b52c7e28cd7b93292b5304d.1556142510.git.liu.denton@gmail.com","subject":"Re: [PATCH v3 3/6] mergetool: use get_merge_tool function","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-04-25T02:36:01Z","receivedAt":"2019-04-25T02:36:09Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Denton Liu writes:\n\n> This change is not completely backwards compatible as there may be\n> external users of git-mergetool--lib. However, only one user,\n> git-diffall[1], was found from searching GitHub and Google. It seems\n> very unlikely that there exists an external caller that would take into\n> account the return code of `get_merge_tool` as it would always return 0\n> before this change so this change probably does not affect any external\n> users.\n\nSounds like a risk that is OK to accept, as it is easy enough to\nreintroduce the middle layer function like you did in the previous\nround when it becomes an issue.\n\nWill queue; thanks.\n"},{"id":"374449","messageId":"xmqq7ebizt1c.fsf@gitster-ct.c.googlers.com","threadId":"50966","inReplyTo":"e975fe4a8b206d8e40f9c4d7cd278fdb5c7358f0.1556142510.git.liu.denton@gmail.com","subject":"Re: [PATCH v3 4/6] mergetool: fallback to tool when guitool unavailable","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-04-25T03:02:23Z","receivedAt":"2019-04-25T03:02:32Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Denton Liu writes:\n\n> In git-difftool, if the tool is called with --gui but `diff.guitool` is\n> not set, it falls back to `diff.tool`. Make git-mergetool also fallback\n> from `merge.guitool` to `merge.tool` if the former is undefined.\n>\n> If git-difftool were to use `get_configured_mergetool`, it would also\n\nI agree that the precedence order below makes sense, but I am a bit\nconfused by \"were to use\" here. Do you mean you'll make the change\nto make difftool to look at mergetool configuraiton in a later step\nin the series? Or is there a way for the user to say \"I want my\ndifftool to also pay attention to the mergetool configurations\" (and\nanother \"I do not want that\" option)? I'll come back to this later.\n\n> get the fallback behaviour in the following precedence:\n>\n> 1. diff.guitool\n> 2. merge.guitool\n> 3. diff.tool\n> 4. merge.tool\n>\n> Signed-off-by: Denton Liu \n> ---\n> Documentation/git-mergetool.txt | 4 +++-\n> git-mergetool--lib.sh | 27 ++++++++++++++++++---------\n> t/t7610-mergetool.sh | 19 +++++++++++++++++++\n> 3 files changed, 40 insertions(+), 10 deletions(-)\n>\n> diff --git a/Documentation/git-mergetool.txt b/Documentation/git-mergetool.txt\n> index 0c7975a050..6b14702e78 100644\n> --- a/Documentation/git-mergetool.txt\n> +++ b/Documentation/git-mergetool.txt\n> @@ -83,7 +83,9 @@ success of the resolution after the custom tool has exited.\n> --gui::\n> \tWhen 'git-mergetool' is invoked with the `-g` or `--gui` option\n> \tthe default merge tool will be read from the configured\n> -\t`merge.guitool` variable instead of `merge.tool`.\n> +\t`merge.guitool` variable instead of `merge.tool`. If\n> +\t`merge.guitool` is not set, we will fallback to the tool\n> +\tconfigured under `merge.tool`.\n\nMakes sense. So \"mergetool --gui\" from the command line would look\nat guitool and then tool in the merge section.\n\n> diff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh\n> index 68ff26a0f7..ada16acffc 100644\n> --- a/git-mergetool--lib.sh\n> +++ b/git-mergetool--lib.sh\n> @@ -350,20 +350,29 @@ guess_merge_tool () {\n> }\n> \n> get_configured_merge_tool () {\n> -\t# If first argument is true, find the guitool instead\n> -\tif test \"$1\" = true\n> +\tis_gui=\"$1\"\n> +\tsections=\"merge\"\n> +\tkeys=\"tool\"\n> +\n> +\tif diff_mode\n> \tthen\n> -\t\tgui_prefix=gui\n> +\t\tsections=\"diff $sections\"\n> \tfi\n> \n> -\t# Diff mode first tries diff.(gui)tool and falls back to merge.(gui)tool.\n> -\t# Merge mode only checks merge.(gui)tool\n> -\tif diff_mode\n> +\tif \"$is_gui\" = true\n> \tthen\n> -\t\tmerge_tool=$(git config diff.${gui_prefix}tool || git config merge.${gui_prefix}tool)\n> -\telse\n> -\t\tmerge_tool=$(git config merge.${gui_prefix}tool)\n> +\t\tkeys=\"guitool $keys\"\n> \tfi\n\nOK, so $sections is \"diff merge\" for difftool and just \"merge\" for mergetool.\n$tool is \"guitool tool\" under \"--gui\", and just \"tool\" otherwise.\n\n> +\tIFS=' '\n> +\tfor key in $keys\n> +\tdo\n> +\t\tfor section in $sections\n> +\t\tdo\n> +\t\t\tmerge_tool=$(git config $section.$key) && break 2\n> +\t\tdone\n> +\tdone\n\nAnd you do up to four iterations to cover the combination in the\nprecedence order. Which makes sense.\n\nI am not sure about the wisdom of setting IFS here, though.\n\nAs far as I can see, both $keys and $sections do not take any\narbitrary values, but just the two (for each) values you know that\ndo not have any funny characters, so I am not sure what you are\ntrying to achieve by that (i.e. benefit is unclear).\n\nAs long as the get_configured_merge_tool function is called always\nfor string_emitted_to_stdout=$(that function), the updated setting\nwill not leak to the outside world so there is no risk to break its\ncallers, but it is not immediately obvious if helper functions\ncalled in the remainder of this function are OK with the modified\nvalue of IFS (i.e. safety is not obvious).\n\nNow for the promised \"come back to this later\", I think you meant\n\"the get_configured_merge_tool function is already prepared to be\nused from difftool in this step and when difftool starts to call it\nhere is what happens\". But I wonder if it makes the evolution of\nthe topic easier to follow if you defer it to a later step when you\nactually make difftool to start calling it? In other words, in this\nstep, your get_configured_merge_tool would look like\n\n\tsections=merge\n\n\tcase \"$1\" in\n\ttrue)\n\t\tkeys=\"guitool tool\" ;;\n\t*)\n\t\tkeys=\"tool\" ;;\n\tesac\n\n\tfor key in $keys\n\tdo\n\t\tfor section in $sections\n\t\tdo\n\t\t\tmerge_tool=$(git config ...) && break 2\n\t\tdone\n\tdone\n\t...\n\nand then in a later step (6/6?), the unconditional setting of\nsections to 'merge' would be updated so that in diff_mode, you'll\niterate over two sections.\n\nI dunno.\n"},{"id":"374450","messageId":"xmqq36m6zsn5.fsf@gitster-ct.c.googlers.com","threadId":"50966","inReplyTo":"f39b15efbd5c765953737f99aa032230f066c074.1556142510.git.liu.denton@gmail.com","subject":"Re: [PATCH v3 6/6] difftool: fallback on merge.guitool","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-04-25T03:10:54Z","receivedAt":"2019-04-25T03:10:59Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Denton Liu writes:\n\n> @@ -734,8 +728,8 @@ int cmd_difftool(int argc, const char **argv, const char *prefix)\n> \tif (use_gui_tool + !!difftool_cmd + !!extcmd > 1)\n> \t\tdie(_(\"--gui, --tool and --extcmd are mutually exclusive\"));\n> \n> -\tif (use_gui_tool && diff_gui_tool && *diff_gui_tool)\n> -\t\tsetenv(\"GIT_DIFF_TOOL\", diff_gui_tool, 1);\n> +\tif (use_gui_tool)\n> +\t\tsetenv(\"GIT_MERGETOOL_GUI\", \"true\", 1);\n> \telse if (difftool_cmd) {\n> \t\tif (*difftool_cmd)\n> \t\t\tsetenv(\"GIT_DIFF_TOOL\", difftool_cmd, 1);\n\nSo unless difftool_cmd is given explicitly, we'll let the scripted\ndifftool--helper to let merge_tool=$(get_merge_tool) to pick the\ntool, which will use the same logic you wrote in the step 2/6.\n\nOK, that makes sense.\n\nWhat was preventing the get_configured_merge_tool updated in step\n2/6 from getting called in difftool was the exporting of\nGIT_DIFF_TOOL we see abovethat was removed by this step.\n\nWhich also makes sense.\n\n> diff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh\n> index 107f31213d..ae90701a12 100755\n> --- a/t/t7800-difftool.sh\n> +++ b/t/t7800-difftool.sh\n> @@ -279,11 +279,27 @@ test_expect_success 'difftool + mergetool config variables' '\n> \techo branch >expect &&\n> \tgit difftool --no-prompt branch >actual &&\n> \ttest_cmp expect actual &&\n> +\tgit difftool --gui --no-prompt branch >actual &&\n> +\ttest_cmp expect actual &&\n> \n> \t# set merge.tool to something bogus, diff.tool to test-tool\n> \ttest_config merge.tool bogus-tool &&\n> \ttest_config diff.tool test-tool &&\n> \tgit difftool --no-prompt branch >actual &&\n> +\ttest_cmp expect actual &&\n> +\tgit difftool --gui --no-prompt branch >actual &&\n> +\ttest_cmp expect actual &&\n> +\n> +\t# set merge.tool, diff.tool to something bogus, merge.guitool to test-tool\n> +\ttest_config diff.tool bogus-tool &&\n> +\ttest_config merge.guitool test-tool &&\n> +\tgit difftool --gui --no-prompt branch >actual &&\n> +\ttest_cmp expect actual &&\n> +\n> +\t# set merge.tool, diff.tool, merge.guitool to something bogus, diff.guitool to test-tool\n> +\ttest_config merge.guitool bogus-tool &&\n> +\ttest_config diff.guitool test-tool &&\n> +\tgit difftool --gui --no-prompt branch >actual &&\n> \ttest_cmp expect actual\n> '\n"},{"id":"374454","messageId":"20190425051620.GA32457@archbookpro.localdomain","threadId":"50966","inReplyTo":"xmqq7ebizt1c.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v3 4/6] mergetool: fallback to tool when guitool unavailable","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2019-04-25T05:16:20Z","receivedAt":"2019-04-25T05:16:26Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"Hi Junio,\n\nOn Thu, Apr 25, 2019 at 12:02:23PM +0900, Junio C Hamano wrote:\n> Denton Liu writes:\n> \n> > In git-difftool, if the tool is called with --gui but `diff.guitool` is\n> > not set, it falls back to `diff.tool`. Make git-mergetool also fallback\n> > from `merge.guitool` to `merge.tool` if the former is undefined.\n> >\n> > If git-difftool were to use `get_configured_mergetool`, it would also\n> \n> I agree that the precedence order below makes sense, but I am a bit\n> confused by \"were to use\" here. Do you mean you'll make the change\n> to make difftool to look at mergetool configuraiton in a later step\n> in the series? Or is there a way for the user to say \"I want my\n> difftool to also pay attention to the mergetool configurations\" (and\n> another \"I do not want that\" option)? I'll come back to this later.\n\nCorrect, it means it'll be done in a future patch (i.e. 6/6).\n\nI guess I wasn't fully clear in the message. I meant something like, \"If\n`git difftool --gui` were to use...\" because difftool currently already\nuses this function in the non-gui case.\n\n[snip]\n> \n> > +\tIFS=' '\n> > +\tfor key in $keys\n> > +\tdo\n> > +\t\tfor section in $sections\n> > +\t\tdo\n> > +\t\t\tmerge_tool=$(git config $section.$key) && break 2\n> > +\t\tdone\n> > +\tdone\n> \n> And you do up to four iterations to cover the combination in the\n> precedence order. Which makes sense.\n> \n> I am not sure about the wisdom of setting IFS here, though.\n> \n> As far as I can see, both $keys and $sections do not take any\n> arbitrary values, but just the two (for each) values you know that\n> do not have any funny characters, so I am not sure what you are\n> trying to achieve by that (i.e. benefit is unclear).\n\nThe reason why IFS is being set is because at the top of mergetool--lib,\nwe set IFS to '\\n'. As a result, without setting IFS, the strings\nwon't parse properly into the for loop.\n\n> \n> As long as the get_configured_merge_tool function is called always\n> for string_emitted_to_stdout=$(that function), the updated setting\n> will not leak to the outside world so there is no risk to break its\n> callers, but it is not immediately obvious if helper functions\n> called in the remainder of this function are OK with the modified\n> value of IFS (i.e. safety is not obvious).\n\nWhen I was writing this, I didn't realise that the value of IFS bleeds\nout of this function. I'll reroll this to use a helper function just in\ncase.\n\n> \n> Now for the promised \"come back to this later\", I think you meant\n> \"the get_configured_merge_tool function is already prepared to be\n> used from difftool in this step and when difftool starts to call it\n> here is what happens\". But I wonder if it makes the evolution of\n> the topic easier to follow if you defer it to a later step when you\n> actually make difftool to start calling it? In other words, in this\n> step, your get_configured_merge_tool would look like\n> \n> \tsections=merge\n> \n> \tcase \"$1\" in\n> \ttrue)\n> \t\tkeys=\"guitool tool\" ;;\n> \t*)\n> \t\tkeys=\"tool\" ;;\n> \tesac\n> \n> \tfor key in $keys\n> \tdo\n> \t\tfor section in $sections\n> \t\tdo\n> \t\t\tmerge_tool=$(git config ...) && break 2\n> \t\tdone\n> \tdone\n> \t...\n> \n> and then in a later step (6/6?), the unconditional setting of\n> sections to 'merge' would be updated so that in diff_mode, you'll\n> iterate over two sections.\n> \n> I dunno.\n\nAs stated above, difftool currently uses this function in the non-gui\ncase. I think that clarifying the log message on my part should make it\neasier to understand the evolution of this topic.\n\nThanks for the careful review,\n\nDenton\n"},{"id":"374478","messageId":"cover.1556185345.git.liu.denton@gmail.com","threadId":"50966","inReplyTo":"cover.1556142510.git.liu.denton@gmail.com","subject":"[PATCH v4 0/6] difftool and mergetool improvements","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2019-04-25T09:54:30Z","receivedAt":"2019-04-25T09:54:36Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"Thanks for another review, Junio.\n\nThis should basically be the same as v3 except I've moved the IFS\nassignment into a subshell so its value does not leak out.\n\nI'd like the change in 4/6 to be reviewed carefully. t7610 and t7800\nrun properly with dash on My Machineة, but I couldn't find a reference\non whether my usage is POSIX legal or not.\n\nWe do have precedent for using fors within a subshell, though, from a\nquick git grep:\n\n\tt/t3202-show-branch-octopus.sh: git show-branch $(for i in $numbers; do echo branch$i; done) > out &&\n\nThanks,\n\nDenton\n\n---\n\nChanges since v3:\n\n* Move nested for into a subshell so that IFS does not leak out of\n function\n\nChanges since v2:\n\n* Unsuppress output in t7610\n* Make `get_merge_tool` return 1 on guessed so we don't have to deal\n with parsing '$guessed:$merge_tool'\n\nChanges since v1:\n\n* Introduce get_merge_tool_guessed function instead of changing\n get_merge_tool\n* Remove unnecessary if-tower in mutual exclusivity logic\nDenton Liu (6):\n t7610: unsuppress output\n t7610: add mergetool --gui tests\n mergetool: use get_merge_tool function\n mergetool: fallback to tool when guitool unavailable\n difftool: make --gui, --tool and --extcmd mutually exclusive\n difftool: fallback on merge.guitool\n\n Documentation/git-difftool.txt | 4 +-\n Documentation/git-mergetool--lib.txt | 4 +-\n Documentation/git-mergetool.txt | 4 +-\n builtin/difftool.c | 13 +--\n git-difftool--helper.sh | 2 +-\n git-mergetool--lib.sh | 37 ++++--\n git-mergetool.sh | 12 +-\n t/t7610-mergetool.sh | 163 +++++++++++++++++----------\n t/t7800-difftool.sh | 24 ++++\n 9 files changed, 172 insertions(+), 91 deletions(-)\n\nRange-diff against v3:\n1: 9f9922cab3 = 1: 9f9922cab3 t7610: unsuppress output\n2: 0f632ca6bf = 2: 0f632ca6bf t7610: add mergetool --gui tests\n3: ff83d25ff2 = 3: ff83d25ff2 mergetool: use get_merge_tool function\n4: e975fe4a8b ! 4: c799e871e2 mergetool: fallback to tool when guitool unavailable\n @@ -6,14 +6,18 @@\n not set, it falls back to `diff.tool`. Make git-mergetool also fallback\n from `merge.guitool` to `merge.tool` if the former is undefined.\n \n - If git-difftool were to use `get_configured_mergetool`, it would also\n - get the fallback behaviour in the following precedence:\n + If git-difftool, when called with `--gui`, were to use\n + `get_configured_mergetool` in a future patch, it would also get the\n + fallback behavior in the following precedence:\n \n 1. diff.guitool\n 2. merge.guitool\n 3. diff.tool\n 4. merge.tool\n \n + Note that the behavior for when difftool or mergetool are called without\n + `--gui` should be identical with or without this patch.\n +\n Signed-off-by: Denton Liu \n \n diff --git a/Documentation/git-mergetool.txt b/Documentation/git-mergetool.txt\n @@ -61,14 +65,19 @@\n +\t\tkeys=\"guitool $keys\"\n \tfi\n +\n -+\tIFS=' '\n -+\tfor key in $keys\n -+\tdo\n -+\t\tfor section in $sections\n ++\tmerge_tool=$(\n ++\t\tIFS=' '\n ++\t\tfor key in $keys\n +\t\tdo\n -+\t\t\tmerge_tool=$(git config $section.$key) && break 2\n -+\t\tdone\n -+\tdone\n ++\t\t\tfor section in $sections\n ++\t\t\tdo\n ++\t\t\t\tif selected=$(git config $section.$key)\n ++\t\t\t\tthen\n ++\t\t\t\t\techo \"$selected\"\n ++\t\t\t\t\treturn\n ++\t\t\t\tfi\n ++\t\t\tdone\n ++\t\tdone)\n +\n \tif test -n \"$merge_tool\" && ! valid_tool \"$merge_tool\"\n \tthen\n5: bc3e229171 = 5: 7d7c936cd3 difftool: make --gui, --tool and --extcmd mutually exclusive\n6: f39b15efbd = 6: b642ed3b1e difftool: fallback on merge.guitool\n-- \n2.21.0.1000.g11cd861522\n\n"},{"id":"374479","messageId":"9f9922cab341396a846edc2f52342c59d294379a.1556185345.git.liu.denton@gmail.com","threadId":"50966","inReplyTo":"cover.1556185345.git.liu.denton@gmail.com","subject":"[PATCH v4 1/6] t7610: unsuppress output","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2019-04-25T09:54:33Z","receivedAt":"2019-04-25T09:54:39Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"The output for commands used to be suppressed by redirecting both stdout\nand stderr to /dev/null. However, this should not happen since the\noutput is useful for debugging and, without the \"-v\" flag, test scripts\ndon't output anyway.\n\nUnsuppress the output by removing the redirections to /dev/null.\n\nSigned-off-by: Denton Liu \n---\n t/t7610-mergetool.sh | 122 +++++++++++++++++++++----------------------\n 1 file changed, 61 insertions(+), 61 deletions(-)\n\ndiff --git a/t/t7610-mergetool.sh b/t/t7610-mergetool.sh\nindex a9fb971615..69711487dd 100755\n--- a/t/t7610-mergetool.sh\n+++ b/t/t7610-mergetool.sh\n@@ -130,14 +130,14 @@ test_expect_success 'custom mergetool' '\n \ttest_when_finished \"git reset --hard\" &&\n \tgit checkout -b test$test_count branch1 &&\n \tgit submodule update -N &&\n-\ttest_must_fail git merge master >/dev/null 2>&1 &&\n-\t( yes \"\" | git mergetool both >/dev/null 2>&1 ) &&\n+\ttest_must_fail git merge master &&\n+\t( yes \"\" | git mergetool both ) &&\n \t( yes \"\" | git mergetool file1 file1 ) &&\n-\t( yes \"\" | git mergetool file2 \"spaced name\" >/dev/null 2>&1 ) &&\n-\t( yes \"\" | git mergetool subdir/file3 >/dev/null 2>&1 ) &&\n-\t( yes \"d\" | git mergetool file11 >/dev/null 2>&1 ) &&\n-\t( yes \"d\" | git mergetool file12 >/dev/null 2>&1 ) &&\n-\t( yes \"l\" | git mergetool submod >/dev/null 2>&1 ) &&\n+\t( yes \"\" | git mergetool file2 \"spaced name\" ) &&\n+\t( yes \"\" | git mergetool subdir/file3 ) &&\n+\t( yes \"d\" | git mergetool file11 ) &&\n+\t( yes \"d\" | git mergetool file12 ) &&\n+\t( yes \"l\" | git mergetool submod ) &&\n \ttest \"$(cat file1)\" = \"master updated\" &&\n \ttest \"$(cat file2)\" = \"master new\" &&\n \ttest \"$(cat subdir/file3)\" = \"master new sub\" &&\n@@ -153,15 +153,15 @@ test_expect_success 'mergetool crlf' '\n \t# test_when_finished is LIFO.)\n \ttest_config core.autocrlf true &&\n \tgit checkout -b test$test_count branch1 &&\n-\ttest_must_fail git merge master >/dev/null 2>&1 &&\n-\t( yes \"\" | git mergetool file1 >/dev/null 2>&1 ) &&\n-\t( yes \"\" | git mergetool file2 >/dev/null 2>&1 ) &&\n-\t( yes \"\" | git mergetool \"spaced name\" >/dev/null 2>&1 ) &&\n-\t( yes \"\" | git mergetool both >/dev/null 2>&1 ) &&\n-\t( yes \"\" | git mergetool subdir/file3 >/dev/null 2>&1 ) &&\n-\t( yes \"d\" | git mergetool file11 >/dev/null 2>&1 ) &&\n-\t( yes \"d\" | git mergetool file12 >/dev/null 2>&1 ) &&\n-\t( yes \"r\" | git mergetool submod >/dev/null 2>&1 ) &&\n+\ttest_must_fail git merge master &&\n+\t( yes \"\" | git mergetool file1 ) &&\n+\t( yes \"\" | git mergetool file2 ) &&\n+\t( yes \"\" | git mergetool \"spaced name\" ) &&\n+\t( yes \"\" | git mergetool both ) &&\n+\t( yes \"\" | git mergetool subdir/file3 ) &&\n+\t( yes \"d\" | git mergetool file11 ) &&\n+\t( yes \"d\" | git mergetool file12 ) &&\n+\t( yes \"r\" | git mergetool submod ) &&\n \ttest \"$(printf x | cat file1 -)\" = \"$(printf \"master updated\\r\\nx\")\" &&\n \ttest \"$(printf x | cat file2 -)\" = \"$(printf \"master new\\r\\nx\")\" &&\n \ttest \"$(printf x | cat subdir/file3 -)\" = \"$(printf \"master new sub\\r\\nx\")\" &&\n@@ -176,8 +176,8 @@ test_expect_success 'mergetool in subdir' '\n \tgit submodule update -N &&\n \t(\n \t\tcd subdir &&\n-\t\ttest_must_fail git merge master >/dev/null 2>&1 &&\n-\t\t( yes \"\" | git mergetool file3 >/dev/null 2>&1 ) &&\n+\t\ttest_must_fail git merge master &&\n+\t\t( yes \"\" | git mergetool file3 ) &&\n \t\ttest \"$(cat file3)\" = \"master new sub\"\n \t)\n '\n@@ -188,14 +188,14 @@ test_expect_success 'mergetool on file in parent dir' '\n \tgit submodule update -N &&\n \t(\n \t\tcd subdir &&\n-\t\ttest_must_fail git merge master >/dev/null 2>&1 &&\n-\t\t( yes \"\" | git mergetool file3 >/dev/null 2>&1 ) &&\n-\t\t( yes \"\" | git mergetool ../file1 >/dev/null 2>&1 ) &&\n-\t\t( yes \"\" | git mergetool ../file2 ../spaced\\ name >/dev/null 2>&1 ) &&\n-\t\t( yes \"\" | git mergetool ../both >/dev/null 2>&1 ) &&\n-\t\t( yes \"d\" | git mergetool ../file11 >/dev/null 2>&1 ) &&\n-\t\t( yes \"d\" | git mergetool ../file12 >/dev/null 2>&1 ) &&\n-\t\t( yes \"l\" | git mergetool ../submod >/dev/null 2>&1 ) &&\n+\t\ttest_must_fail git merge master &&\n+\t\t( yes \"\" | git mergetool file3 ) &&\n+\t\t( yes \"\" | git mergetool ../file1 ) &&\n+\t\t( yes \"\" | git mergetool ../file2 ../spaced\\ name ) &&\n+\t\t( yes \"\" | git mergetool ../both ) &&\n+\t\t( yes \"d\" | git mergetool ../file11 ) &&\n+\t\t( yes \"d\" | git mergetool ../file12 ) &&\n+\t\t( yes \"l\" | git mergetool ../submod ) &&\n \t\ttest \"$(cat ../file1)\" = \"master updated\" &&\n \t\ttest \"$(cat ../file2)\" = \"master new\" &&\n \t\ttest \"$(cat ../submod/bar)\" = \"branch1 submodule\" &&\n@@ -209,9 +209,9 @@ test_expect_success 'mergetool skips autoresolved' '\n \tgit submodule update -N &&\n \ttest_must_fail git merge master &&\n \ttest -n \"$(git ls-files -u)\" &&\n-\t( yes \"d\" | git mergetool file11 >/dev/null 2>&1 ) &&\n-\t( yes \"d\" | git mergetool file12 >/dev/null 2>&1 ) &&\n-\t( yes \"l\" | git mergetool submod >/dev/null 2>&1 ) &&\n+\t( yes \"d\" | git mergetool file11 ) &&\n+\t( yes \"d\" | git mergetool file12 ) &&\n+\t( yes \"l\" | git mergetool submod ) &&\n \toutput=\"$(git mergetool --no-prompt)\" &&\n \ttest \"$output\" = \"No files need merging\"\n '\n@@ -259,9 +259,9 @@ test_expect_success 'mergetool skips resolved paths when rerere is active' '\n \trm -rf .git/rr-cache &&\n \tgit checkout -b test$test_count branch1 &&\n \tgit submodule update -N &&\n-\ttest_must_fail git merge master >/dev/null 2>&1 &&\n-\t( yes \"l\" | git mergetool --no-prompt submod >/dev/null 2>&1 ) &&\n-\t( yes \"d\" \"d\" | git mergetool --no-prompt >/dev/null 2>&1 ) &&\n+\ttest_must_fail git merge master &&\n+\t( yes \"l\" | git mergetool --no-prompt submod ) &&\n+\t( yes \"d\" \"d\" | git mergetool --no-prompt ) &&\n \tgit submodule update -N &&\n \toutput=\"$(yes \"n\" | git mergetool --no-prompt)\" &&\n \ttest \"$output\" = \"No files need merging\"\n@@ -369,9 +369,9 @@ test_expect_success 'deleted vs modified submodule' '\n \tgit checkout -b test$test_count.a test$test_count &&\n \ttest_must_fail git merge master &&\n \ttest -n \"$(git ls-files -u)\" &&\n-\t( yes \"\" | git mergetool file1 file2 spaced\\ name subdir/file3 >/dev/null 2>&1 ) &&\n-\t( yes \"\" | git mergetool both >/dev/null 2>&1 ) &&\n-\t( yes \"d\" | git mergetool file11 file12 >/dev/null 2>&1 ) &&\n+\t( yes \"\" | git mergetool file1 file2 spaced\\ name subdir/file3 ) &&\n+\t( yes \"\" | git mergetool both ) &&\n+\t( yes \"d\" | git mergetool file11 file12 ) &&\n \t( yes \"r\" | git mergetool submod ) &&\n \trmdir submod && mv submod-movedaside submod &&\n \ttest \"$(cat submod/bar)\" = \"branch1 submodule\" &&\n@@ -386,9 +386,9 @@ test_expect_success 'deleted vs modified submodule' '\n \tgit submodule update -N &&\n \ttest_must_fail git merge master &&\n \ttest -n \"$(git ls-files -u)\" &&\n-\t( yes \"\" | git mergetool file1 file2 spaced\\ name subdir/file3 >/dev/null 2>&1 ) &&\n-\t( yes \"\" | git mergetool both >/dev/null 2>&1 ) &&\n-\t( yes \"d\" | git mergetool file11 file12 >/dev/null 2>&1 ) &&\n+\t( yes \"\" | git mergetool file1 file2 spaced\\ name subdir/file3 ) &&\n+\t( yes \"\" | git mergetool both ) &&\n+\t( yes \"d\" | git mergetool file11 file12 ) &&\n \t( yes \"l\" | git mergetool submod ) &&\n \ttest ! -e submod &&\n \toutput=\"$(git mergetool --no-prompt)\" &&\n@@ -400,9 +400,9 @@ test_expect_success 'deleted vs modified submodule' '\n \tgit submodule update -N &&\n \ttest_must_fail git merge test$test_count &&\n \ttest -n \"$(git ls-files -u)\" &&\n-\t( yes \"\" | git mergetool file1 file2 spaced\\ name subdir/file3 >/dev/null 2>&1 ) &&\n-\t( yes \"\" | git mergetool both >/dev/null 2>&1 ) &&\n-\t( yes \"d\" | git mergetool file11 file12 >/dev/null 2>&1 ) &&\n+\t( yes \"\" | git mergetool file1 file2 spaced\\ name subdir/file3 ) &&\n+\t( yes \"\" | git mergetool both ) &&\n+\t( yes \"d\" | git mergetool file11 file12 ) &&\n \t( yes \"r\" | git mergetool submod ) &&\n \ttest ! -e submod &&\n \ttest -d submod.orig &&\n@@ -416,9 +416,9 @@ test_expect_success 'deleted vs modified submodule' '\n \tgit submodule update -N &&\n \ttest_must_fail git merge test$test_count &&\n \ttest -n \"$(git ls-files -u)\" &&\n-\t( yes \"\" | git mergetool file1 file2 spaced\\ name subdir/file3 >/dev/null 2>&1 ) &&\n-\t( yes \"\" | git mergetool both >/dev/null 2>&1 ) &&\n-\t( yes \"d\" | git mergetool file11 file12 >/dev/null 2>&1 ) &&\n+\t( yes \"\" | git mergetool file1 file2 spaced\\ name subdir/file3 ) &&\n+\t( yes \"\" | git mergetool both ) &&\n+\t( yes \"d\" | git mergetool file11 file12 ) &&\n \t( yes \"l\" | git mergetool submod ) &&\n \ttest \"$(cat submod/bar)\" = \"master submodule\" &&\n \tgit submodule update -N &&\n@@ -440,9 +440,9 @@ test_expect_success 'file vs modified submodule' '\n \tgit checkout -b test$test_count.a branch1 &&\n \ttest_must_fail git merge master &&\n \ttest -n \"$(git ls-files -u)\" &&\n-\t( yes \"\" | git mergetool file1 file2 spaced\\ name subdir/file3 >/dev/null 2>&1 ) &&\n-\t( yes \"\" | git mergetool both >/dev/null 2>&1 ) &&\n-\t( yes \"d\" | git mergetool file11 file12 >/dev/null 2>&1 ) &&\n+\t( yes \"\" | git mergetool file1 file2 spaced\\ name subdir/file3 ) &&\n+\t( yes \"\" | git mergetool both ) &&\n+\t( yes \"d\" | git mergetool file11 file12 ) &&\n \t( yes \"r\" | git mergetool submod ) &&\n \trmdir submod && mv submod-movedaside submod &&\n \ttest \"$(cat submod/bar)\" = \"branch1 submodule\" &&\n@@ -456,9 +456,9 @@ test_expect_success 'file vs modified submodule' '\n \tgit checkout -b test$test_count.b test$test_count &&\n \ttest_must_fail git merge master &&\n \ttest -n \"$(git ls-files -u)\" &&\n-\t( yes \"\" | git mergetool file1 file2 spaced\\ name subdir/file3 >/dev/null 2>&1 ) &&\n-\t( yes \"\" | git mergetool both >/dev/null 2>&1 ) &&\n-\t( yes \"d\" | git mergetool file11 file12 >/dev/null 2>&1 ) &&\n+\t( yes \"\" | git mergetool file1 file2 spaced\\ name subdir/file3 ) &&\n+\t( yes \"\" | git mergetool both ) &&\n+\t( yes \"d\" | git mergetool file11 file12 ) &&\n \t( yes \"l\" | git mergetool submod ) &&\n \tgit submodule update -N &&\n \ttest \"$(cat submod)\" = \"not a submodule\" &&\n@@ -472,9 +472,9 @@ test_expect_success 'file vs modified submodule' '\n \tgit submodule update -N &&\n \ttest_must_fail git merge test$test_count &&\n \ttest -n \"$(git ls-files -u)\" &&\n-\t( yes \"\" | git mergetool file1 file2 spaced\\ name subdir/file3 >/dev/null 2>&1 ) &&\n-\t( yes \"\" | git mergetool both >/dev/null 2>&1 ) &&\n-\t( yes \"d\" | git mergetool file11 file12 >/dev/null 2>&1 ) &&\n+\t( yes \"\" | git mergetool file1 file2 spaced\\ name subdir/file3 ) &&\n+\t( yes \"\" | git mergetool both ) &&\n+\t( yes \"d\" | git mergetool file11 file12 ) &&\n \t( yes \"r\" | git mergetool submod ) &&\n \ttest -d submod.orig &&\n \tgit submodule update -N &&\n@@ -488,9 +488,9 @@ test_expect_success 'file vs modified submodule' '\n \tgit submodule update -N &&\n \ttest_must_fail git merge test$test_count &&\n \ttest -n \"$(git ls-files -u)\" &&\n-\t( yes \"\" | git mergetool file1 file2 spaced\\ name subdir/file3 >/dev/null 2>&1 ) &&\n-\t( yes \"\" | git mergetool both>/dev/null 2>&1 ) &&\n-\t( yes \"d\" | git mergetool file11 file12 >/dev/null 2>&1 ) &&\n+\t( yes \"\" | git mergetool file1 file2 spaced\\ name subdir/file3 ) &&\n+\t( yes \"\" | git mergetool both ) &&\n+\t( yes \"d\" | git mergetool file11 file12 ) &&\n \t( yes \"l\" | git mergetool submod ) &&\n \ttest \"$(cat submod/bar)\" = \"master submodule\" &&\n \tgit submodule update -N &&\n@@ -543,7 +543,7 @@ test_expect_success 'submodule in subdirectory' '\n \tgit add subdir/subdir_module &&\n \tgit commit -m \"change submodule in subdirectory on test$test_count.b\" &&\n \n-\ttest_must_fail git merge test$test_count.a >/dev/null 2>&1 &&\n+\ttest_must_fail git merge test$test_count.a &&\n \t(\n \t\tcd subdir &&\n \t\t( yes \"l\" | git mergetool subdir_module )\n@@ -554,7 +554,7 @@ test_expect_success 'submodule in subdirectory' '\n \tgit reset --hard &&\n \tgit submodule update -N &&\n \n-\ttest_must_fail git merge test$test_count.a >/dev/null 2>&1 &&\n+\ttest_must_fail git merge test$test_count.a &&\n \t( yes \"r\" | git mergetool subdir/subdir_module ) &&\n \ttest \"$(cat subdir/subdir_module/file15)\" = \"test$test_count.b\" &&\n \tgit submodule update -N &&\n@@ -641,7 +641,7 @@ test_expect_success 'filenames seen by tools start with ./' '\n \ttest_config mergetool.myecho.trustExitCode true &&\n \ttest_must_fail git merge master &&\n \tgit mergetool --no-prompt --tool myecho -- both >actual &&\n-\tgrep ^\\./both_LOCAL_ actual >/dev/null\n+\tgrep ^\\./both_LOCAL_ actual\n '\n \n test_lazy_prereq MKTEMP '\n@@ -658,8 +658,8 @@ test_expect_success MKTEMP 'temporary filenames are used with mergetool.writeToT\n \ttest_config mergetool.myecho.trustExitCode true &&\n \ttest_must_fail git merge master &&\n \tgit mergetool --no-prompt --tool myecho -- both >actual &&\n-\t! grep ^\\./both_LOCAL_ actual >/dev/null &&\n-\tgrep /both_LOCAL_ actual >/dev/null\n+\t! grep ^\\./both_LOCAL_ actual &&\n+\tgrep /both_LOCAL_ actual\n '\n \n test_expect_success 'diff.orderFile configuration is honored' '\n-- \n2.21.0.1000.g11cd861522\n\n"},{"id":"374480","messageId":"0f632ca6bf4f5e76d18b17435a8e565b4cdc3cc0.1556185345.git.liu.denton@gmail.com","threadId":"50966","inReplyTo":"cover.1556185345.git.liu.denton@gmail.com","subject":"[PATCH v4 2/6] t7610: add mergetool --gui tests","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2019-04-25T09:54:36Z","receivedAt":"2019-04-25T09:54:41Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"In 063f2bdbf7 (mergetool: accept -g/--[no-]gui as arguments,\n2018-10-24), mergetool was taught the --gui option but no tests were\nadded to ensure that it was working properly. Add a test to ensure that\nit works.\n\nSigned-off-by: Denton Liu \n---\n t/t7610-mergetool.sh | 22 ++++++++++++++++++++++\n 1 file changed, 22 insertions(+)\n\ndiff --git a/t/t7610-mergetool.sh b/t/t7610-mergetool.sh\nindex 69711487dd..dad607e186 100755\n--- a/t/t7610-mergetool.sh\n+++ b/t/t7610-mergetool.sh\n@@ -145,6 +145,28 @@ test_expect_success 'custom mergetool' '\n \tgit commit -m \"branch1 resolved with mergetool\"\n '\n \n+test_expect_success 'gui mergetool' '\n+\ttest_config merge.guitool myguitool &&\n+\ttest_config mergetool.myguitool.cmd \"(printf \\\"gui \\\" && cat \\\"\\$REMOTE\\\") >\\\"\\$MERGED\\\"\" &&\n+\ttest_config mergetool.myguitool.trustExitCode true &&\n+\ttest_when_finished \"git reset --hard\" &&\n+\tgit checkout -b test$test_count branch1 &&\n+\tgit submodule update -N &&\n+\ttest_must_fail git merge master &&\n+\t( yes \"\" | git mergetool --gui both ) &&\n+\t( yes \"\" | git mergetool -g file1 file1 ) &&\n+\t( yes \"\" | git mergetool --gui file2 \"spaced name\" ) &&\n+\t( yes \"\" | git mergetool --gui subdir/file3 ) &&\n+\t( yes \"d\" | git mergetool --gui file11 ) &&\n+\t( yes \"d\" | git mergetool --gui file12 ) &&\n+\t( yes \"l\" | git mergetool --gui submod ) &&\n+\ttest \"$(cat file1)\" = \"gui master updated\" &&\n+\ttest \"$(cat file2)\" = \"gui master new\" &&\n+\ttest \"$(cat subdir/file3)\" = \"gui master new sub\" &&\n+\ttest \"$(cat submod/bar)\" = \"branch1 submodule\" &&\n+\tgit commit -m \"branch1 resolved with mergetool\"\n+'\n+\n test_expect_success 'mergetool crlf' '\n \ttest_when_finished \"git reset --hard\" &&\n \t# This test_config line must go after the above reset line so that\n-- \n2.21.0.1000.g11cd861522\n\n"},{"id":"374481","messageId":"ff83d25ff2d0b9585b52c7e28cd7b93292b5304d.1556185345.git.liu.denton@gmail.com","threadId":"50966","inReplyTo":"cover.1556185345.git.liu.denton@gmail.com","subject":"[PATCH v4 3/6] mergetool: use get_merge_tool function","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2019-04-25T09:54:39Z","receivedAt":"2019-04-25T09:54:44Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"In git-mergetool, the logic for getting which merge tool to use is\nduplicated in git-mergetool--lib, except for the fact that it needs to\nknow whether the tool was guessed or not.\n\nRewrite `get_merge_tool` to return whether or not the tool was guessed\nthrough the return code and make git-mergetool call this function\ninstead of duplicating the logic. Note that 1 was chosen to be the\nreturn code of when a tool is guessed because it seems like a slightly\nmore abnormal condition than getting a tool that's explicitly specified\nbut this is completely arbitrary.\n\nAlso, let `$GIT_MERGETOOL_GUI` be set to determine whether or not the\nguitool will be selected.\n\nThis change is not completely backwards compatible as there may be\nexternal users of git-mergetool--lib. However, only one user,\ngit-diffall[1], was found from searching GitHub and Google. It seems\nvery unlikely that there exists an external caller that would take into\naccount the return code of `get_merge_tool` as it would always return 0\nbefore this change so this change probably does not affect any external\nusers.\n\n[1]: https://github.com/thenigan/git-diffall\n\nSigned-off-by: Denton Liu \n---\n Documentation/git-mergetool--lib.txt | 4 +++-\n git-difftool--helper.sh | 2 +-\n git-mergetool--lib.sh | 5 ++++-\n git-mergetool.sh | 12 ++++--------\n 4 files changed, 12 insertions(+), 11 deletions(-)\n\ndiff --git a/Documentation/git-mergetool--lib.txt b/Documentation/git-mergetool--lib.txt\nindex 055550b2bc..4da9d24096 100644\n--- a/Documentation/git-mergetool--lib.txt\n+++ b/Documentation/git-mergetool--lib.txt\n@@ -28,7 +28,9 @@ to define the operation mode for the functions listed below.\n FUNCTIONS\n ---------\n get_merge_tool::\n-\treturns a merge tool.\n+\treturns a merge tool. the return code is 1 if we returned a guessed\n+\tmerge tool, else 0. '$GIT_MERGETOOL_GUI' may be set to 'true' to\n+\tsearch for the appropriate guitool.\n \n get_merge_tool_cmd::\n \treturns the custom command for a merge tool.\ndiff --git a/git-difftool--helper.sh b/git-difftool--helper.sh\nindex 7bfb6737df..46af3e60b7 100755\n--- a/git-difftool--helper.sh\n+++ b/git-difftool--helper.sh\n@@ -71,7 +71,7 @@ then\n \tthen\n \t\tmerge_tool=\"$GIT_DIFF_TOOL\"\n \telse\n-\t\tmerge_tool=\"$(get_merge_tool)\" || exit\n+\t\tmerge_tool=\"$(get_merge_tool)\"\n \tfi\n fi\n \ndiff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh\nindex 83bf52494c..68ff26a0f7 100644\n--- a/git-mergetool--lib.sh\n+++ b/git-mergetool--lib.sh\n@@ -403,14 +403,17 @@ get_merge_tool_path () {\n }\n \n get_merge_tool () {\n+\tnot_guessed=true\n \t# Check if a merge tool has been configured\n-\tmerge_tool=$(get_configured_merge_tool)\n+\tmerge_tool=$(get_configured_merge_tool $GIT_MERGETOOL_GUI)\n \t# Try to guess an appropriate merge tool if no tool has been set.\n \tif test -z \"$merge_tool\"\n \tthen\n \t\tmerge_tool=$(guess_merge_tool) || exit\n+\t\tnot_guessed=false\n \tfi\n \techo \"$merge_tool\"\n+\ttest \"$not_guessed\" = true\n }\n \n mergetool_find_win32_cmd () {\ndiff --git a/git-mergetool.sh b/git-mergetool.sh\nindex 01b9ad59b2..88fa6a914a 100755\n--- a/git-mergetool.sh\n+++ b/git-mergetool.sh\n@@ -389,7 +389,7 @@ print_noop_and_exit () {\n \n main () {\n \tprompt=$(git config --bool mergetool.prompt)\n-\tgui_tool=false\n+\tGIT_MERGETOOL_GUI=false\n \tguessed_merge_tool=false\n \torderfile=\n \n@@ -416,10 +416,10 @@ main () {\n \t\t\tesac\n \t\t\t;;\n \t\t--no-gui)\n-\t\t\tgui_tool=false\n+\t\t\tGIT_MERGETOOL_GUI=false\n \t\t\t;;\n \t\t-g|--gui)\n-\t\t\tgui_tool=true\n+\t\t\tGIT_MERGETOOL_GUI=true\n \t\t\t;;\n \t\t-y|--no-prompt)\n \t\t\tprompt=false\n@@ -449,12 +449,8 @@ main () {\n \n \tif test -z \"$merge_tool\"\n \tthen\n-\t\t# Check if a merge tool has been configured\n-\t\tmerge_tool=$(get_configured_merge_tool $gui_tool)\n-\t\t# Try to guess an appropriate merge tool if no tool has been set.\n-\t\tif test -z \"$merge_tool\"\n+\t\tif ! merge_tool=$(get_merge_tool)\n \t\tthen\n-\t\t\tmerge_tool=$(guess_merge_tool) || exit\n \t\t\tguessed_merge_tool=true\n \t\tfi\n \tfi\n-- \n2.21.0.1000.g11cd861522\n\n"},{"id":"374482","messageId":"c799e871e2a6a6a7fcca45aad71f5a0c406ba3d7.1556185345.git.liu.denton@gmail.com","threadId":"50966","inReplyTo":"cover.1556185345.git.liu.denton@gmail.com","subject":"[PATCH v4 4/6] mergetool: fallback to tool when guitool unavailable","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2019-04-25T09:54:41Z","receivedAt":"2019-04-25T09:54:48Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"In git-difftool, if the tool is called with --gui but `diff.guitool` is\nnot set, it falls back to `diff.tool`. Make git-mergetool also fallback\nfrom `merge.guitool` to `merge.tool` if the former is undefined.\n\nIf git-difftool, when called with `--gui`, were to use\n`get_configured_mergetool` in a future patch, it would also get the\nfallback behavior in the following precedence:\n\n1. diff.guitool\n2. merge.guitool\n3. diff.tool\n4. merge.tool\n\nNote that the behavior for when difftool or mergetool are called without\n`--gui` should be identical with or without this patch.\n\nSigned-off-by: Denton Liu \n---\n Documentation/git-mergetool.txt | 4 +++-\n git-mergetool--lib.sh | 32 +++++++++++++++++++++++---------\n t/t7610-mergetool.sh | 19 +++++++++++++++++++\n 3 files changed, 45 insertions(+), 10 deletions(-)\n\ndiff --git a/Documentation/git-mergetool.txt b/Documentation/git-mergetool.txt\nindex 0c7975a050..6b14702e78 100644\n--- a/Documentation/git-mergetool.txt\n+++ b/Documentation/git-mergetool.txt\n@@ -83,7 +83,9 @@ success of the resolution after the custom tool has exited.\n --gui::\n \tWhen 'git-mergetool' is invoked with the `-g` or `--gui` option\n \tthe default merge tool will be read from the configured\n-\t`merge.guitool` variable instead of `merge.tool`.\n+\t`merge.guitool` variable instead of `merge.tool`. If\n+\t`merge.guitool` is not set, we will fallback to the tool\n+\tconfigured under `merge.tool`.\n \n --no-gui::\n \tThis overrides a previous `-g` or `--gui` setting and reads the\ndiff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh\nindex 68ff26a0f7..c4b16c5e59 100644\n--- a/git-mergetool--lib.sh\n+++ b/git-mergetool--lib.sh\n@@ -350,20 +350,34 @@ guess_merge_tool () {\n }\n \n get_configured_merge_tool () {\n-\t# If first argument is true, find the guitool instead\n-\tif test \"$1\" = true\n+\tis_gui=\"$1\"\n+\tsections=\"merge\"\n+\tkeys=\"tool\"\n+\n+\tif diff_mode\n \tthen\n-\t\tgui_prefix=gui\n+\t\tsections=\"diff $sections\"\n \tfi\n \n-\t# Diff mode first tries diff.(gui)tool and falls back to merge.(gui)tool.\n-\t# Merge mode only checks merge.(gui)tool\n-\tif diff_mode\n+\tif \"$is_gui\" = true\n \tthen\n-\t\tmerge_tool=$(git config diff.${gui_prefix}tool || git config merge.${gui_prefix}tool)\n-\telse\n-\t\tmerge_tool=$(git config merge.${gui_prefix}tool)\n+\t\tkeys=\"guitool $keys\"\n \tfi\n+\n+\tmerge_tool=$(\n+\t\tIFS=' '\n+\t\tfor key in $keys\n+\t\tdo\n+\t\t\tfor section in $sections\n+\t\t\tdo\n+\t\t\t\tif selected=$(git config $section.$key)\n+\t\t\t\tthen\n+\t\t\t\t\techo \"$selected\"\n+\t\t\t\t\treturn\n+\t\t\t\tfi\n+\t\t\tdone\n+\t\tdone)\n+\n \tif test -n \"$merge_tool\" && ! valid_tool \"$merge_tool\"\n \tthen\n \t\techo >&2 \"git config option $TOOL_MODE.${gui_prefix}tool set to unknown tool: $merge_tool\"\ndiff --git a/t/t7610-mergetool.sh b/t/t7610-mergetool.sh\nindex dad607e186..5b61c10a9c 100755\n--- a/t/t7610-mergetool.sh\n+++ b/t/t7610-mergetool.sh\n@@ -167,6 +167,25 @@ test_expect_success 'gui mergetool' '\n \tgit commit -m \"branch1 resolved with mergetool\"\n '\n \n+test_expect_success 'gui mergetool without merge.guitool set falls back to merge.tool' '\n+\ttest_when_finished \"git reset --hard\" &&\n+\tgit checkout -b test$test_count branch1 &&\n+\tgit submodule update -N &&\n+\ttest_must_fail git merge master &&\n+\t( yes \"\" | git mergetool --gui both ) &&\n+\t( yes \"\" | git mergetool -g file1 file1 ) &&\n+\t( yes \"\" | git mergetool --gui file2 \"spaced name\" ) &&\n+\t( yes \"\" | git mergetool --gui subdir/file3 ) &&\n+\t( yes \"d\" | git mergetool --gui file11 ) &&\n+\t( yes \"d\" | git mergetool --gui file12 ) &&\n+\t( yes \"l\" | git mergetool --gui submod ) &&\n+\ttest \"$(cat file1)\" = \"master updated\" &&\n+\ttest \"$(cat file2)\" = \"master new\" &&\n+\ttest \"$(cat subdir/file3)\" = \"master new sub\" &&\n+\ttest \"$(cat submod/bar)\" = \"branch1 submodule\" &&\n+\tgit commit -m \"branch1 resolved with mergetool\"\n+'\n+\n test_expect_success 'mergetool crlf' '\n \ttest_when_finished \"git reset --hard\" &&\n \t# This test_config line must go after the above reset line so that\n-- \n2.21.0.1000.g11cd861522\n\n"},{"id":"374483","messageId":"7d7c936cd3dabe95c2b26d46c5b5bb980c26586b.1556185345.git.liu.denton@gmail.com","threadId":"50966","inReplyTo":"cover.1556185345.git.liu.denton@gmail.com","subject":"[PATCH v4 5/6] difftool: make --gui, --tool and --extcmd mutually exclusive","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2019-04-25T09:54:44Z","receivedAt":"2019-04-25T09:54:50Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"In git-difftool, these options specify which tool to ultimately run. As\na result, they are logically conflicting. Explicitly disallow these\noptions from being used together.\n\nSigned-off-by: Denton Liu \n---\n builtin/difftool.c | 3 +++\n t/t7800-difftool.sh | 8 ++++++++\n 2 files changed, 11 insertions(+)\n\ndiff --git a/builtin/difftool.c b/builtin/difftool.c\nindex a3ea60ea71..65bba90338 100644\n--- a/builtin/difftool.c\n+++ b/builtin/difftool.c\n@@ -731,6 +731,9 @@ int cmd_difftool(int argc, const char **argv, const char *prefix)\n \tsetenv(GIT_DIR_ENVIRONMENT, absolute_path(get_git_dir()), 1);\n \tsetenv(GIT_WORK_TREE_ENVIRONMENT, absolute_path(get_git_work_tree()), 1);\n \n+\tif (use_gui_tool + !!difftool_cmd + !!extcmd > 1)\n+\t\tdie(_(\"--gui, --tool and --extcmd are mutually exclusive\"));\n+\n \tif (use_gui_tool && diff_gui_tool && *diff_gui_tool)\n \t\tsetenv(\"GIT_DIFF_TOOL\", diff_gui_tool, 1);\n \telse if (difftool_cmd) {\ndiff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh\nindex bb9a7f4ff9..107f31213d 100755\n--- a/t/t7800-difftool.sh\n+++ b/t/t7800-difftool.sh\n@@ -705,4 +705,12 @@ test_expect_success SYMLINKS 'difftool --dir-diff handles modified symlinks' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'difftool --gui, --tool and --extcmd are exclusive' '\n+\tdifftool_test_setup &&\n+\ttest_must_fail git difftool --gui --tool=test-tool &&\n+\ttest_must_fail git difftool --gui --extcmd=cat &&\n+\ttest_must_fail git difftool --tool=test-tool --extcmd=cat &&\n+\ttest_must_fail git difftool --gui --tool=test-tool --extcmd=cat\n+'\n+\n test_done\n-- \n2.21.0.1000.g11cd861522\n\n"},{"id":"374484","messageId":"b642ed3b1e8e19ebc0566c10b033e50d8183581d.1556185345.git.liu.denton@gmail.com","threadId":"50966","inReplyTo":"cover.1556185345.git.liu.denton@gmail.com","subject":"[PATCH v4 6/6] difftool: fallback on merge.guitool","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2019-04-25T09:54:47Z","receivedAt":"2019-04-25T09:54:53Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"In git-difftool.txt, it says\n\n\t'git difftool' falls back to 'git mergetool' config variables when the\n\tdifftool equivalents have not been defined.\n\nHowever, when `diff.guitool` is missing, it doesn't fallback to\nanything. Make git-difftool fallback to `merge.guitool` when `diff.guitool` is\nmissing.\n\nSigned-off-by: Denton Liu \n---\n Documentation/git-difftool.txt | 4 +++-\n builtin/difftool.c | 10 ++--------\n t/t7800-difftool.sh | 16 ++++++++++++++++\n 3 files changed, 21 insertions(+), 9 deletions(-)\n\ndiff --git a/Documentation/git-difftool.txt b/Documentation/git-difftool.txt\nindex 96c26e6aa8..484c485fd0 100644\n--- a/Documentation/git-difftool.txt\n+++ b/Documentation/git-difftool.txt\n@@ -90,7 +90,9 @@ instead. `--no-symlinks` is the default on Windows.\n \tWhen 'git-difftool' is invoked with the `-g` or `--gui` option\n \tthe default diff tool will be read from the configured\n \t`diff.guitool` variable instead of `diff.tool`. The `--no-gui`\n-\toption can be used to override this setting.\n+\toption can be used to override this setting. If `diff.guitool`\n+\tis not set, we will fallback in the order of `merge.guitool`,\n+\t`diff.tool`, `merge.tool` until a tool is found.\n \n --[no-]trust-exit-code::\n \t'git-difftool' invokes a diff tool individually on each file.\ndiff --git a/builtin/difftool.c b/builtin/difftool.c\nindex 65bba90338..10660639c0 100644\n--- a/builtin/difftool.c\n+++ b/builtin/difftool.c\n@@ -24,7 +24,6 @@\n #include \"object-store.h\"\n #include \"dir.h\"\n \n-static char *diff_gui_tool;\n static int trust_exit_code;\n \n static const char *const builtin_difftool_usage[] = {\n@@ -34,11 +33,6 @@ static const char *const builtin_difftool_usage[] = {\n \n static int difftool_config(const char *var, const char *value, void *cb)\n {\n-\tif (!strcmp(var, \"diff.guitool\")) {\n-\t\tdiff_gui_tool = xstrdup(value);\n-\t\treturn 0;\n-\t}\n-\n \tif (!strcmp(var, \"difftool.trustexitcode\")) {\n \t\ttrust_exit_code = git_config_bool(var, value);\n \t\treturn 0;\n@@ -734,8 +728,8 @@ int cmd_difftool(int argc, const char **argv, const char *prefix)\n \tif (use_gui_tool + !!difftool_cmd + !!extcmd > 1)\n \t\tdie(_(\"--gui, --tool and --extcmd are mutually exclusive\"));\n \n-\tif (use_gui_tool && diff_gui_tool && *diff_gui_tool)\n-\t\tsetenv(\"GIT_DIFF_TOOL\", diff_gui_tool, 1);\n+\tif (use_gui_tool)\n+\t\tsetenv(\"GIT_MERGETOOL_GUI\", \"true\", 1);\n \telse if (difftool_cmd) {\n \t\tif (*difftool_cmd)\n \t\t\tsetenv(\"GIT_DIFF_TOOL\", difftool_cmd, 1);\ndiff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh\nindex 107f31213d..ae90701a12 100755\n--- a/t/t7800-difftool.sh\n+++ b/t/t7800-difftool.sh\n@@ -279,11 +279,27 @@ test_expect_success 'difftool + mergetool config variables' '\n \techo branch >expect &&\n \tgit difftool --no-prompt branch >actual &&\n \ttest_cmp expect actual &&\n+\tgit difftool --gui --no-prompt branch >actual &&\n+\ttest_cmp expect actual &&\n \n \t# set merge.tool to something bogus, diff.tool to test-tool\n \ttest_config merge.tool bogus-tool &&\n \ttest_config diff.tool test-tool &&\n \tgit difftool --no-prompt branch >actual &&\n+\ttest_cmp expect actual &&\n+\tgit difftool --gui --no-prompt branch >actual &&\n+\ttest_cmp expect actual &&\n+\n+\t# set merge.tool, diff.tool to something bogus, merge.guitool to test-tool\n+\ttest_config diff.tool bogus-tool &&\n+\ttest_config merge.guitool test-tool &&\n+\tgit difftool --gui --no-prompt branch >actual &&\n+\ttest_cmp expect actual &&\n+\n+\t# set merge.tool, diff.tool, merge.guitool to something bogus, diff.guitool to test-tool\n+\ttest_config merge.guitool bogus-tool &&\n+\ttest_config diff.guitool test-tool &&\n+\tgit difftool --gui --no-prompt branch >actual &&\n \ttest_cmp expect actual\n '\n \n-- \n2.21.0.1000.g11cd861522\n\n"},{"id":"374614","messageId":"20190428235256.GA6418@gmail.com","threadId":"50966","inReplyTo":"ff83d25ff2d0b9585b52c7e28cd7b93292b5304d.1556185345.git.liu.denton@gmail.com","subject":"Re: [PATCH v4 3/6] mergetool: use get_merge_tool function","fromName":"David Aguilar","fromEmail":"davvid@gmail.com","sentAt":"2019-04-28T23:52:56Z","receivedAt":"2019-04-28T23:53:03Z","isPatch":true,"sender":{"key":"davvid@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13196?v=4"},"body":"On Thu, Apr 25, 2019 at 02:54:39AM -0700, Denton Liu wrote:\n> In git-mergetool, the logic for getting which merge tool to use is\n> duplicated in git-mergetool--lib, except for the fact that it needs to\n> know whether the tool was guessed or not.\n> \n> Rewrite `get_merge_tool` to return whether or not the tool was guessed\n> through the return code and make git-mergetool call this function\n> instead of duplicating the logic. Note that 1 was chosen to be the\n> return code of when a tool is guessed because it seems like a slightly\n> more abnormal condition than getting a tool that's explicitly specified\n> but this is completely arbitrary.\n> \n> Also, let `$GIT_MERGETOOL_GUI` be set to determine whether or not the\n> guitool will be selected.\n> \n> This change is not completely backwards compatible as there may be\n> external users of git-mergetool--lib. However, only one user,\n> git-diffall[1], was found from searching GitHub and Google. It seems\n> very unlikely that there exists an external caller that would take into\n> account the return code of `get_merge_tool` as it would always return 0\n> before this change so this change probably does not affect any external\n> users.\n> \n> [1]: https://github.com/thenigan/git-diffall\n> \n> Signed-off-by: Denton Liu \n\n\nThe author of git-diffall approached the list some time ago and his\ncontribution resulted in \"git difftool --dir-diff\".\n\nThese days we would probably encourage users to use the difftool\nbuilt-in feature rather than \"diffall\", but thank you for your\ncareful consideration against breaking external scripts.\n\nMaybe we can submit a PR against the diffall repo mentioning\nthe backstory so that new users are pointed to the main tool.\n\n\n> diff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh\n> index 83bf52494c..68ff26a0f7 100644\n> --- a/git-mergetool--lib.sh\n> +++ b/git-mergetool--lib.sh\n> @@ -403,14 +403,17 @@ get_merge_tool_path () {\n> }\n> \n> get_merge_tool () {\n> +\tnot_guessed=true\n\nTiny nit; I find double-negatives hard to understand. Maybe rename this\nto \"is_guessed\" and flip the logic below so that we can keep the\nvariable named as a positive flag?\n\n\n> \t# Check if a merge tool has been configured\n> -\tmerge_tool=$(get_configured_merge_tool)\n> +\tmerge_tool=$(get_configured_merge_tool $GIT_MERGETOOL_GUI)\n> \t# Try to guess an appropriate merge tool if no tool has been set.\n> \tif test -z \"$merge_tool\"\n> \tthen\n> \t\tmerge_tool=$(guess_merge_tool) || exit\n> +\t\tnot_guessed=false\n> \tfi\n> \techo \"$merge_tool\"\n> +\ttest \"$not_guessed\" = true\n> }\n\n-- \nDavid\n"},{"id":"374615","messageId":"20190428235624.GA5304@gmail.com","threadId":"50966","inReplyTo":"c799e871e2a6a6a7fcca45aad71f5a0c406ba3d7.1556185345.git.liu.denton@gmail.com","subject":"Re: [PATCH v4 4/6] mergetool: fallback to tool when guitool unavailable","fromName":"David Aguilar","fromEmail":"davvid@gmail.com","sentAt":"2019-04-28T23:56:24Z","receivedAt":"2019-04-28T23:56:29Z","isPatch":true,"sender":{"key":"davvid@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13196?v=4"},"body":"On Thu, Apr 25, 2019 at 02:54:41AM -0700, Denton Liu wrote:\n> diff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh\n> index 68ff26a0f7..c4b16c5e59 100644\n> --- a/git-mergetool--lib.sh\n> +++ b/git-mergetool--lib.sh\n> @@ -350,20 +350,34 @@ guess_merge_tool () {\n> }\n> \n> get_configured_merge_tool () {\n> -\t# If first argument is true, find the guitool instead\n> -\tif test \"$1\" = true\n> +\tis_gui=\"$1\"\n> +\tsections=\"merge\"\n> +\tkeys=\"tool\"\n> +\n> +\tif diff_mode\n> \tthen\n> -\t\tgui_prefix=gui\n> +\t\tsections=\"diff $sections\"\n> \tfi\n> \n> -\t# Diff mode first tries diff.(gui)tool and falls back to merge.(gui)tool.\n> -\t# Merge mode only checks merge.(gui)tool\n> -\tif diff_mode\n> +\tif \"$is_gui\" = true\n\nThis line looks suspect. How about,\n\n\tif test \"$is_gui\" = true\n\ninstead? This expression could also be lifted out to an \"is_gui\"\nhelper function.\n\n\n> \tthen\n> -\t\tmerge_tool=$(git config diff.${gui_prefix}tool || git config merge.${gui_prefix}tool)\n> -\telse\n> -\t\tmerge_tool=$(git config merge.${gui_prefix}tool)\n> +\t\tkeys=\"guitool $keys\"\n> \tfi\n> +\n> +\tmerge_tool=$(\n> +\t\tIFS=' '\n> +\t\tfor key in $keys\n> +\t\tdo\n> +\t\t\tfor section in $sections\n> +\t\t\tdo\n> +\t\t\t\tif selected=$(git config $section.$key)\n\nWould it be simpler to split this conditional into two lines?\n\n\tselected=$(git config ...)\n\tif test -n \"$selected\"\n\tthen\n\t\t...\n\tfi\n\nYes, it stops looking at the exit code, but it instead focuses on the\nresult, which is slightly more bulletproof against a funky user\nconfiguration.\n\n\nRegarding the two loops above, what would it look like if we\nunrolled the logic and just spelled out the keys up front that it's a\nlittle easier to follow?\n\nI agree it is nicer from an implementation sense to use loops,\nbut we really shouldn't be planning to extend to more permutations in\nthe future beyond the diff/merge duality, so being explicit and spelling\nout each config lookup permutation is simpler to understand since we\nonly have 4 states. We should be discouraged from adding any more ;-)\n\nSomething like,\n\n\tkeys=\n\tif merge_mode\n\tthen\n\t\tif gui_mode # probably worth adding this function\n\t\tthen\n\t\t\tkeys=\"merge.guitool merge.tool\"\n\t\telse\n\t\t\tkeys=\"merge.tool\"\n\t\tfi\n\telse\n\t\tif gui_mode\n\t\tthen\n\t\t\tkeys=\"diff.guitool merge.guitool diff.tool merge.tool\"\n\t\telse\n\t\t\tkeys=\"diff.tool merge.tool\"\n\t\tfi\n\tfi\n\n.. and then just have a single loop over $keys.\n-- \nDavid\n"},{"id":"374620","messageId":"cover.1556518203.git.liu.denton@gmail.com","threadId":"50966","inReplyTo":"cover.1556185345.git.liu.denton@gmail.com","subject":"[PATCH v5 0/7] difftool and mergetool improvements","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2019-04-29T06:20:58Z","receivedAt":"2019-04-29T06:21:05Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"\nHi David, thanks for the comments.\n\nI'm not really sure why I did the double-negative thing... It seems\nobvious that it should be the other way around. I also unrolled the\nloops and wrote a gui_mode function.\n\nGood suggestions!\n\n---\n\nChanges since v4:\n\n* Remove double-negative\n* Change double-nested search loop into one for-loop\n* Create gui_mode function\n* Change an instance of \"exclusive\" to \"mutually exclusive\"\n\nChanges since v3:\n\n* Move nested for into a subshell so that IFS does not leak out of\n function\n\nChanges since v2:\n\n* Unsuppress output in t7610\n* Make `get_merge_tool` return 1 on guessed so we don't have to deal\n with parsing '$guessed:$merge_tool'\n\nChanges since v1:\n\n* Introduce get_merge_tool_guessed function instead of changing\n get_merge_tool\n* Remove unnecessary if-tower in mutual exclusivity logic\n\nDenton Liu (7):\n t7610: unsuppress output\n t7610: add mergetool --gui tests\n mergetool: use get_merge_tool function\n mergetool--lib: create gui_mode function\n mergetool: fallback to tool when guitool unavailable\n difftool: make --gui, --tool and --extcmd mutually exclusive\n difftool: fallback on merge.guitool\n\n Documentation/git-difftool.txt | 4 +-\n Documentation/git-mergetool--lib.txt | 4 +-\n Documentation/git-mergetool.txt | 4 +-\n builtin/difftool.c | 13 +--\n git-difftool--helper.sh | 2 +-\n git-mergetool--lib.sh | 47 ++++++--\n git-mergetool.sh | 12 +-\n t/t7610-mergetool.sh | 163 +++++++++++++++++----------\n t/t7800-difftool.sh | 24 ++++\n 9 files changed, 180 insertions(+), 93 deletions(-)\n\nRange-diff against v4:\n1: 919aa32e20 = 1: 9f9922cab3 t7610: unsuppress output\n2: 9a1bb60b20 = 2: 0f632ca6bf t7610: add mergetool --gui tests\n3: a900ce2a6a ! 3: 81dd25d8e2 mergetool: use get_merge_tool function\n @@ -18,8 +18,9 @@\n \n This change is not completely backwards compatible as there may be\n external users of git-mergetool--lib. However, only one user,\n - git-diffall[1], was found from searching GitHub and Google. It seems\n - very unlikely that there exists an external caller that would take into\n + git-diffall[1], was found from searching GitHub and Google, and this\n + tool is superseded by `git difftool --dir-diff` anyway. It seems very\n + unlikely that there exists an external caller that would take into\n account the return code of `get_merge_tool` as it would always return 0\n before this change so this change probably does not affect any external\n users.\n @@ -63,7 +64,7 @@\n }\n \n get_merge_tool () {\n -+\tnot_guessed=true\n ++\tis_guessed=false\n \t# Check if a merge tool has been configured\n -\tmerge_tool=$(get_configured_merge_tool)\n +\tmerge_tool=$(get_configured_merge_tool $GIT_MERGETOOL_GUI)\n @@ -71,10 +72,10 @@\n \tif test -z \"$merge_tool\"\n \tthen\n \t\tmerge_tool=$(guess_merge_tool) || exit\n -+\t\tnot_guessed=false\n ++\t\tis_guessed=true\n \tfi\n \techo \"$merge_tool\"\n -+\ttest \"$not_guessed\" = true\n ++\ttest \"$is_guessed\" = false\n }\n \n mergetool_find_win32_cmd () {\n-: ---------- > 4: 27a59e1e27 mergetool--lib: create gui_mode function\n4: abcf91688a ! 5: 40413dbda1 mergetool: fallback to tool when guitool unavailable\n @@ -15,8 +15,41 @@\n 3. diff.tool\n 4. merge.tool\n \n - Note that the behavior for when difftool or mergetool are called without\n - `--gui` should be identical with or without this patch.\n + The behavior for when difftool or mergetool are called without `--gui`\n + should be identical with or without this patch.\n +\n + Note that the search loop could be written as\n +\n + sections=\"merge\"\n + keys=\"tool\"\n + if diff_mode\n + then\n + sections=\"diff $sections\"\n + fi\n + if gui_mode\n + then\n + keys=\"guitool $keys\"\n + fi\n +\n + merge_tool=$(\n + IFS=' '\n + for key in $keys\n + do\n + for section in $sections\n + do\n + selected=$(git config $section.$key)\n + if test -n \"$selected\"\n + then\n + echo \"$selected\"\n + return\n + fi\n + done\n + done)\n +\n + which would make adding a mode in the future much easier. However,\n + adding a new mode will likely never happen as it is highly discouraged\n + so, as a result, it is written in its current form so that it's\n + immediately obvious for future readers.\n \n Signed-off-by: Denton Liu \n \n @@ -42,41 +75,43 @@\n }\n \n get_configured_merge_tool () {\n --\t# If first argument is true, find the guitool instead\n --\tif test \"$1\" = true\n -+\tis_gui=\"$1\"\n -+\tsections=\"merge\"\n -+\tkeys=\"tool\"\n -+\n -+\tif diff_mode\n - \tthen\n +-\tif gui_mode\n +-\tthen\n -\t\tgui_prefix=gui\n -+\t\tsections=\"diff $sections\"\n - \tfi\n - \n +-\tfi\n +-\n -\t# Diff mode first tries diff.(gui)tool and falls back to merge.(gui)tool.\n -\t# Merge mode only checks merge.(gui)tool\n --\tif diff_mode\n -+\tif \"$is_gui\" = true\n ++\tkeys=\n + \tif diff_mode\n \tthen\n -\t\tmerge_tool=$(git config diff.${gui_prefix}tool || git config merge.${gui_prefix}tool)\n --\telse\n ++\t\tif gui_mode\n ++\t\tthen\n ++\t\t\tkeys=\"diff.guitool merge.guitool diff.tool merge.tool\"\n ++\t\telse\n ++\t\t\tkeys=\"diff.tool merge.tool\"\n ++\t\tfi\n + \telse\n -\t\tmerge_tool=$(git config merge.${gui_prefix}tool)\n -+\t\tkeys=\"guitool $keys\"\n ++\t\tif gui_mode\n ++\t\tthen\n ++\t\t\tkeys=\"merge.guitool merge.tool\"\n ++\t\telse\n ++\t\t\tkeys=\"merge.tool\"\n ++\t\tfi\n \tfi\n +\n +\tmerge_tool=$(\n +\t\tIFS=' '\n +\t\tfor key in $keys\n +\t\tdo\n -+\t\t\tfor section in $sections\n -+\t\t\tdo\n -+\t\t\t\tif selected=$(git config $section.$key)\n -+\t\t\t\tthen\n -+\t\t\t\t\techo \"$selected\"\n -+\t\t\t\t\treturn\n -+\t\t\t\tfi\n -+\t\t\tdone\n ++\t\t\tselected=$(git config $key)\n ++\t\t\tif test -n \"$selected\"\n ++\t\t\tthen\n ++\t\t\t\techo \"$selected\"\n ++\t\t\t\treturn\n ++\t\t\tfi\n +\t\tdone)\n +\n \tif test -n \"$merge_tool\" && ! valid_tool \"$merge_tool\"\n5: 9ec39c5af0 ! 6: c70789b689 difftool: make --gui, --tool and --extcmd mutually exclusive\n @@ -29,7 +29,7 @@\n \ttest_cmp expect actual\n '\n \n -+test_expect_success 'difftool --gui, --tool and --extcmd are exclusive' '\n ++test_expect_success 'difftool --gui, --tool and --extcmd are mutually exclusive' '\n +\tdifftool_test_setup &&\n +\ttest_must_fail git difftool --gui --tool=test-tool &&\n +\ttest_must_fail git difftool --gui --extcmd=cat &&\n6: a72009fc3d = 7: 3fd4f46a7c difftool: fallback on merge.guitool\n-- \n2.21.0.1033.g0e8cc1100c\n\n"},{"id":"374621","messageId":"9f9922cab341396a846edc2f52342c59d294379a.1556518203.git.liu.denton@gmail.com","threadId":"50966","inReplyTo":"cover.1556518203.git.liu.denton@gmail.com","subject":"[PATCH v5 1/7] t7610: unsuppress output","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2019-04-29T06:21:02Z","receivedAt":"2019-04-29T06:21:08Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"The output for commands used to be suppressed by redirecting both stdout\nand stderr to /dev/null. However, this should not happen since the\noutput is useful for debugging and, without the \"-v\" flag, test scripts\ndon't output anyway.\n\nUnsuppress the output by removing the redirections to /dev/null.\n\nSigned-off-by: Denton Liu \n---\n t/t7610-mergetool.sh | 122 +++++++++++++++++++++----------------------\n 1 file changed, 61 insertions(+), 61 deletions(-)\n\ndiff --git a/t/t7610-mergetool.sh b/t/t7610-mergetool.sh\nindex a9fb971615..69711487dd 100755\n--- a/t/t7610-mergetool.sh\n+++ b/t/t7610-mergetool.sh\n@@ -130,14 +130,14 @@ test_expect_success 'custom mergetool' '\n \ttest_when_finished \"git reset --hard\" &&\n \tgit checkout -b test$test_count branch1 &&\n \tgit submodule update -N &&\n-\ttest_must_fail git merge master >/dev/null 2>&1 &&\n-\t( yes \"\" | git mergetool both >/dev/null 2>&1 ) &&\n+\ttest_must_fail git merge master &&\n+\t( yes \"\" | git mergetool both ) &&\n \t( yes \"\" | git mergetool file1 file1 ) &&\n-\t( yes \"\" | git mergetool file2 \"spaced name\" >/dev/null 2>&1 ) &&\n-\t( yes \"\" | git mergetool subdir/file3 >/dev/null 2>&1 ) &&\n-\t( yes \"d\" | git mergetool file11 >/dev/null 2>&1 ) &&\n-\t( yes \"d\" | git mergetool file12 >/dev/null 2>&1 ) &&\n-\t( yes \"l\" | git mergetool submod >/dev/null 2>&1 ) &&\n+\t( yes \"\" | git mergetool file2 \"spaced name\" ) &&\n+\t( yes \"\" | git mergetool subdir/file3 ) &&\n+\t( yes \"d\" | git mergetool file11 ) &&\n+\t( yes \"d\" | git mergetool file12 ) &&\n+\t( yes \"l\" | git mergetool submod ) &&\n \ttest \"$(cat file1)\" = \"master updated\" &&\n \ttest \"$(cat file2)\" = \"master new\" &&\n \ttest \"$(cat subdir/file3)\" = \"master new sub\" &&\n@@ -153,15 +153,15 @@ test_expect_success 'mergetool crlf' '\n \t# test_when_finished is LIFO.)\n \ttest_config core.autocrlf true &&\n \tgit checkout -b test$test_count branch1 &&\n-\ttest_must_fail git merge master >/dev/null 2>&1 &&\n-\t( yes \"\" | git mergetool file1 >/dev/null 2>&1 ) &&\n-\t( yes \"\" | git mergetool file2 >/dev/null 2>&1 ) &&\n-\t( yes \"\" | git mergetool \"spaced name\" >/dev/null 2>&1 ) &&\n-\t( yes \"\" | git mergetool both >/dev/null 2>&1 ) &&\n-\t( yes \"\" | git mergetool subdir/file3 >/dev/null 2>&1 ) &&\n-\t( yes \"d\" | git mergetool file11 >/dev/null 2>&1 ) &&\n-\t( yes \"d\" | git mergetool file12 >/dev/null 2>&1 ) &&\n-\t( yes \"r\" | git mergetool submod >/dev/null 2>&1 ) &&\n+\ttest_must_fail git merge master &&\n+\t( yes \"\" | git mergetool file1 ) &&\n+\t( yes \"\" | git mergetool file2 ) &&\n+\t( yes \"\" | git mergetool \"spaced name\" ) &&\n+\t( yes \"\" | git mergetool both ) &&\n+\t( yes \"\" | git mergetool subdir/file3 ) &&\n+\t( yes \"d\" | git mergetool file11 ) &&\n+\t( yes \"d\" | git mergetool file12 ) &&\n+\t( yes \"r\" | git mergetool submod ) &&\n \ttest \"$(printf x | cat file1 -)\" = \"$(printf \"master updated\\r\\nx\")\" &&\n \ttest \"$(printf x | cat file2 -)\" = \"$(printf \"master new\\r\\nx\")\" &&\n \ttest \"$(printf x | cat subdir/file3 -)\" = \"$(printf \"master new sub\\r\\nx\")\" &&\n@@ -176,8 +176,8 @@ test_expect_success 'mergetool in subdir' '\n \tgit submodule update -N &&\n \t(\n \t\tcd subdir &&\n-\t\ttest_must_fail git merge master >/dev/null 2>&1 &&\n-\t\t( yes \"\" | git mergetool file3 >/dev/null 2>&1 ) &&\n+\t\ttest_must_fail git merge master &&\n+\t\t( yes \"\" | git mergetool file3 ) &&\n \t\ttest \"$(cat file3)\" = \"master new sub\"\n \t)\n '\n@@ -188,14 +188,14 @@ test_expect_success 'mergetool on file in parent dir' '\n \tgit submodule update -N &&\n \t(\n \t\tcd subdir &&\n-\t\ttest_must_fail git merge master >/dev/null 2>&1 &&\n-\t\t( yes \"\" | git mergetool file3 >/dev/null 2>&1 ) &&\n-\t\t( yes \"\" | git mergetool ../file1 >/dev/null 2>&1 ) &&\n-\t\t( yes \"\" | git mergetool ../file2 ../spaced\\ name >/dev/null 2>&1 ) &&\n-\t\t( yes \"\" | git mergetool ../both >/dev/null 2>&1 ) &&\n-\t\t( yes \"d\" | git mergetool ../file11 >/dev/null 2>&1 ) &&\n-\t\t( yes \"d\" | git mergetool ../file12 >/dev/null 2>&1 ) &&\n-\t\t( yes \"l\" | git mergetool ../submod >/dev/null 2>&1 ) &&\n+\t\ttest_must_fail git merge master &&\n+\t\t( yes \"\" | git mergetool file3 ) &&\n+\t\t( yes \"\" | git mergetool ../file1 ) &&\n+\t\t( yes \"\" | git mergetool ../file2 ../spaced\\ name ) &&\n+\t\t( yes \"\" | git mergetool ../both ) &&\n+\t\t( yes \"d\" | git mergetool ../file11 ) &&\n+\t\t( yes \"d\" | git mergetool ../file12 ) &&\n+\t\t( yes \"l\" | git mergetool ../submod ) &&\n \t\ttest \"$(cat ../file1)\" = \"master updated\" &&\n \t\ttest \"$(cat ../file2)\" = \"master new\" &&\n \t\ttest \"$(cat ../submod/bar)\" = \"branch1 submodule\" &&\n@@ -209,9 +209,9 @@ test_expect_success 'mergetool skips autoresolved' '\n \tgit submodule update -N &&\n \ttest_must_fail git merge master &&\n \ttest -n \"$(git ls-files -u)\" &&\n-\t( yes \"d\" | git mergetool file11 >/dev/null 2>&1 ) &&\n-\t( yes \"d\" | git mergetool file12 >/dev/null 2>&1 ) &&\n-\t( yes \"l\" | git mergetool submod >/dev/null 2>&1 ) &&\n+\t( yes \"d\" | git mergetool file11 ) &&\n+\t( yes \"d\" | git mergetool file12 ) &&\n+\t( yes \"l\" | git mergetool submod ) &&\n \toutput=\"$(git mergetool --no-prompt)\" &&\n \ttest \"$output\" = \"No files need merging\"\n '\n@@ -259,9 +259,9 @@ test_expect_success 'mergetool skips resolved paths when rerere is active' '\n \trm -rf .git/rr-cache &&\n \tgit checkout -b test$test_count branch1 &&\n \tgit submodule update -N &&\n-\ttest_must_fail git merge master >/dev/null 2>&1 &&\n-\t( yes \"l\" | git mergetool --no-prompt submod >/dev/null 2>&1 ) &&\n-\t( yes \"d\" \"d\" | git mergetool --no-prompt >/dev/null 2>&1 ) &&\n+\ttest_must_fail git merge master &&\n+\t( yes \"l\" | git mergetool --no-prompt submod ) &&\n+\t( yes \"d\" \"d\" | git mergetool --no-prompt ) &&\n \tgit submodule update -N &&\n \toutput=\"$(yes \"n\" | git mergetool --no-prompt)\" &&\n \ttest \"$output\" = \"No files need merging\"\n@@ -369,9 +369,9 @@ test_expect_success 'deleted vs modified submodule' '\n \tgit checkout -b test$test_count.a test$test_count &&\n \ttest_must_fail git merge master &&\n \ttest -n \"$(git ls-files -u)\" &&\n-\t( yes \"\" | git mergetool file1 file2 spaced\\ name subdir/file3 >/dev/null 2>&1 ) &&\n-\t( yes \"\" | git mergetool both >/dev/null 2>&1 ) &&\n-\t( yes \"d\" | git mergetool file11 file12 >/dev/null 2>&1 ) &&\n+\t( yes \"\" | git mergetool file1 file2 spaced\\ name subdir/file3 ) &&\n+\t( yes \"\" | git mergetool both ) &&\n+\t( yes \"d\" | git mergetool file11 file12 ) &&\n \t( yes \"r\" | git mergetool submod ) &&\n \trmdir submod && mv submod-movedaside submod &&\n \ttest \"$(cat submod/bar)\" = \"branch1 submodule\" &&\n@@ -386,9 +386,9 @@ test_expect_success 'deleted vs modified submodule' '\n \tgit submodule update -N &&\n \ttest_must_fail git merge master &&\n \ttest -n \"$(git ls-files -u)\" &&\n-\t( yes \"\" | git mergetool file1 file2 spaced\\ name subdir/file3 >/dev/null 2>&1 ) &&\n-\t( yes \"\" | git mergetool both >/dev/null 2>&1 ) &&\n-\t( yes \"d\" | git mergetool file11 file12 >/dev/null 2>&1 ) &&\n+\t( yes \"\" | git mergetool file1 file2 spaced\\ name subdir/file3 ) &&\n+\t( yes \"\" | git mergetool both ) &&\n+\t( yes \"d\" | git mergetool file11 file12 ) &&\n \t( yes \"l\" | git mergetool submod ) &&\n \ttest ! -e submod &&\n \toutput=\"$(git mergetool --no-prompt)\" &&\n@@ -400,9 +400,9 @@ test_expect_success 'deleted vs modified submodule' '\n \tgit submodule update -N &&\n \ttest_must_fail git merge test$test_count &&\n \ttest -n \"$(git ls-files -u)\" &&\n-\t( yes \"\" | git mergetool file1 file2 spaced\\ name subdir/file3 >/dev/null 2>&1 ) &&\n-\t( yes \"\" | git mergetool both >/dev/null 2>&1 ) &&\n-\t( yes \"d\" | git mergetool file11 file12 >/dev/null 2>&1 ) &&\n+\t( yes \"\" | git mergetool file1 file2 spaced\\ name subdir/file3 ) &&\n+\t( yes \"\" | git mergetool both ) &&\n+\t( yes \"d\" | git mergetool file11 file12 ) &&\n \t( yes \"r\" | git mergetool submod ) &&\n \ttest ! -e submod &&\n \ttest -d submod.orig &&\n@@ -416,9 +416,9 @@ test_expect_success 'deleted vs modified submodule' '\n \tgit submodule update -N &&\n \ttest_must_fail git merge test$test_count &&\n \ttest -n \"$(git ls-files -u)\" &&\n-\t( yes \"\" | git mergetool file1 file2 spaced\\ name subdir/file3 >/dev/null 2>&1 ) &&\n-\t( yes \"\" | git mergetool both >/dev/null 2>&1 ) &&\n-\t( yes \"d\" | git mergetool file11 file12 >/dev/null 2>&1 ) &&\n+\t( yes \"\" | git mergetool file1 file2 spaced\\ name subdir/file3 ) &&\n+\t( yes \"\" | git mergetool both ) &&\n+\t( yes \"d\" | git mergetool file11 file12 ) &&\n \t( yes \"l\" | git mergetool submod ) &&\n \ttest \"$(cat submod/bar)\" = \"master submodule\" &&\n \tgit submodule update -N &&\n@@ -440,9 +440,9 @@ test_expect_success 'file vs modified submodule' '\n \tgit checkout -b test$test_count.a branch1 &&\n \ttest_must_fail git merge master &&\n \ttest -n \"$(git ls-files -u)\" &&\n-\t( yes \"\" | git mergetool file1 file2 spaced\\ name subdir/file3 >/dev/null 2>&1 ) &&\n-\t( yes \"\" | git mergetool both >/dev/null 2>&1 ) &&\n-\t( yes \"d\" | git mergetool file11 file12 >/dev/null 2>&1 ) &&\n+\t( yes \"\" | git mergetool file1 file2 spaced\\ name subdir/file3 ) &&\n+\t( yes \"\" | git mergetool both ) &&\n+\t( yes \"d\" | git mergetool file11 file12 ) &&\n \t( yes \"r\" | git mergetool submod ) &&\n \trmdir submod && mv submod-movedaside submod &&\n \ttest \"$(cat submod/bar)\" = \"branch1 submodule\" &&\n@@ -456,9 +456,9 @@ test_expect_success 'file vs modified submodule' '\n \tgit checkout -b test$test_count.b test$test_count &&\n \ttest_must_fail git merge master &&\n \ttest -n \"$(git ls-files -u)\" &&\n-\t( yes \"\" | git mergetool file1 file2 spaced\\ name subdir/file3 >/dev/null 2>&1 ) &&\n-\t( yes \"\" | git mergetool both >/dev/null 2>&1 ) &&\n-\t( yes \"d\" | git mergetool file11 file12 >/dev/null 2>&1 ) &&\n+\t( yes \"\" | git mergetool file1 file2 spaced\\ name subdir/file3 ) &&\n+\t( yes \"\" | git mergetool both ) &&\n+\t( yes \"d\" | git mergetool file11 file12 ) &&\n \t( yes \"l\" | git mergetool submod ) &&\n \tgit submodule update -N &&\n \ttest \"$(cat submod)\" = \"not a submodule\" &&\n@@ -472,9 +472,9 @@ test_expect_success 'file vs modified submodule' '\n \tgit submodule update -N &&\n \ttest_must_fail git merge test$test_count &&\n \ttest -n \"$(git ls-files -u)\" &&\n-\t( yes \"\" | git mergetool file1 file2 spaced\\ name subdir/file3 >/dev/null 2>&1 ) &&\n-\t( yes \"\" | git mergetool both >/dev/null 2>&1 ) &&\n-\t( yes \"d\" | git mergetool file11 file12 >/dev/null 2>&1 ) &&\n+\t( yes \"\" | git mergetool file1 file2 spaced\\ name subdir/file3 ) &&\n+\t( yes \"\" | git mergetool both ) &&\n+\t( yes \"d\" | git mergetool file11 file12 ) &&\n \t( yes \"r\" | git mergetool submod ) &&\n \ttest -d submod.orig &&\n \tgit submodule update -N &&\n@@ -488,9 +488,9 @@ test_expect_success 'file vs modified submodule' '\n \tgit submodule update -N &&\n \ttest_must_fail git merge test$test_count &&\n \ttest -n \"$(git ls-files -u)\" &&\n-\t( yes \"\" | git mergetool file1 file2 spaced\\ name subdir/file3 >/dev/null 2>&1 ) &&\n-\t( yes \"\" | git mergetool both>/dev/null 2>&1 ) &&\n-\t( yes \"d\" | git mergetool file11 file12 >/dev/null 2>&1 ) &&\n+\t( yes \"\" | git mergetool file1 file2 spaced\\ name subdir/file3 ) &&\n+\t( yes \"\" | git mergetool both ) &&\n+\t( yes \"d\" | git mergetool file11 file12 ) &&\n \t( yes \"l\" | git mergetool submod ) &&\n \ttest \"$(cat submod/bar)\" = \"master submodule\" &&\n \tgit submodule update -N &&\n@@ -543,7 +543,7 @@ test_expect_success 'submodule in subdirectory' '\n \tgit add subdir/subdir_module &&\n \tgit commit -m \"change submodule in subdirectory on test$test_count.b\" &&\n \n-\ttest_must_fail git merge test$test_count.a >/dev/null 2>&1 &&\n+\ttest_must_fail git merge test$test_count.a &&\n \t(\n \t\tcd subdir &&\n \t\t( yes \"l\" | git mergetool subdir_module )\n@@ -554,7 +554,7 @@ test_expect_success 'submodule in subdirectory' '\n \tgit reset --hard &&\n \tgit submodule update -N &&\n \n-\ttest_must_fail git merge test$test_count.a >/dev/null 2>&1 &&\n+\ttest_must_fail git merge test$test_count.a &&\n \t( yes \"r\" | git mergetool subdir/subdir_module ) &&\n \ttest \"$(cat subdir/subdir_module/file15)\" = \"test$test_count.b\" &&\n \tgit submodule update -N &&\n@@ -641,7 +641,7 @@ test_expect_success 'filenames seen by tools start with ./' '\n \ttest_config mergetool.myecho.trustExitCode true &&\n \ttest_must_fail git merge master &&\n \tgit mergetool --no-prompt --tool myecho -- both >actual &&\n-\tgrep ^\\./both_LOCAL_ actual >/dev/null\n+\tgrep ^\\./both_LOCAL_ actual\n '\n \n test_lazy_prereq MKTEMP '\n@@ -658,8 +658,8 @@ test_expect_success MKTEMP 'temporary filenames are used with mergetool.writeToT\n \ttest_config mergetool.myecho.trustExitCode true &&\n \ttest_must_fail git merge master &&\n \tgit mergetool --no-prompt --tool myecho -- both >actual &&\n-\t! grep ^\\./both_LOCAL_ actual >/dev/null &&\n-\tgrep /both_LOCAL_ actual >/dev/null\n+\t! grep ^\\./both_LOCAL_ actual &&\n+\tgrep /both_LOCAL_ actual\n '\n \n test_expect_success 'diff.orderFile configuration is honored' '\n-- \n2.21.0.1033.g0e8cc1100c\n\n"},{"id":"374622","messageId":"0f632ca6bf4f5e76d18b17435a8e565b4cdc3cc0.1556518203.git.liu.denton@gmail.com","threadId":"50966","inReplyTo":"cover.1556518203.git.liu.denton@gmail.com","subject":"[PATCH v5 2/7] t7610: add mergetool --gui tests","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2019-04-29T06:21:05Z","receivedAt":"2019-04-29T06:21:10Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"In 063f2bdbf7 (mergetool: accept -g/--[no-]gui as arguments,\n2018-10-24), mergetool was taught the --gui option but no tests were\nadded to ensure that it was working properly. Add a test to ensure that\nit works.\n\nSigned-off-by: Denton Liu \n---\n t/t7610-mergetool.sh | 22 ++++++++++++++++++++++\n 1 file changed, 22 insertions(+)\n\ndiff --git a/t/t7610-mergetool.sh b/t/t7610-mergetool.sh\nindex 69711487dd..dad607e186 100755\n--- a/t/t7610-mergetool.sh\n+++ b/t/t7610-mergetool.sh\n@@ -145,6 +145,28 @@ test_expect_success 'custom mergetool' '\n \tgit commit -m \"branch1 resolved with mergetool\"\n '\n \n+test_expect_success 'gui mergetool' '\n+\ttest_config merge.guitool myguitool &&\n+\ttest_config mergetool.myguitool.cmd \"(printf \\\"gui \\\" && cat \\\"\\$REMOTE\\\") >\\\"\\$MERGED\\\"\" &&\n+\ttest_config mergetool.myguitool.trustExitCode true &&\n+\ttest_when_finished \"git reset --hard\" &&\n+\tgit checkout -b test$test_count branch1 &&\n+\tgit submodule update -N &&\n+\ttest_must_fail git merge master &&\n+\t( yes \"\" | git mergetool --gui both ) &&\n+\t( yes \"\" | git mergetool -g file1 file1 ) &&\n+\t( yes \"\" | git mergetool --gui file2 \"spaced name\" ) &&\n+\t( yes \"\" | git mergetool --gui subdir/file3 ) &&\n+\t( yes \"d\" | git mergetool --gui file11 ) &&\n+\t( yes \"d\" | git mergetool --gui file12 ) &&\n+\t( yes \"l\" | git mergetool --gui submod ) &&\n+\ttest \"$(cat file1)\" = \"gui master updated\" &&\n+\ttest \"$(cat file2)\" = \"gui master new\" &&\n+\ttest \"$(cat subdir/file3)\" = \"gui master new sub\" &&\n+\ttest \"$(cat submod/bar)\" = \"branch1 submodule\" &&\n+\tgit commit -m \"branch1 resolved with mergetool\"\n+'\n+\n test_expect_success 'mergetool crlf' '\n \ttest_when_finished \"git reset --hard\" &&\n \t# This test_config line must go after the above reset line so that\n-- \n2.21.0.1033.g0e8cc1100c\n\n"},{"id":"374623","messageId":"81dd25d8e2bac8c22c018fd35532aaab36526e74.1556518203.git.liu.denton@gmail.com","threadId":"50966","inReplyTo":"cover.1556518203.git.liu.denton@gmail.com","subject":"[PATCH v5 3/7] mergetool: use get_merge_tool function","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2019-04-29T06:21:08Z","receivedAt":"2019-04-29T06:21:14Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"In git-mergetool, the logic for getting which merge tool to use is\nduplicated in git-mergetool--lib, except for the fact that it needs to\nknow whether the tool was guessed or not.\n\nRewrite `get_merge_tool` to return whether or not the tool was guessed\nthrough the return code and make git-mergetool call this function\ninstead of duplicating the logic. Note that 1 was chosen to be the\nreturn code of when a tool is guessed because it seems like a slightly\nmore abnormal condition than getting a tool that's explicitly specified\nbut this is completely arbitrary.\n\nAlso, let `$GIT_MERGETOOL_GUI` be set to determine whether or not the\nguitool will be selected.\n\nThis change is not completely backwards compatible as there may be\nexternal users of git-mergetool--lib. However, only one user,\ngit-diffall[1], was found from searching GitHub and Google, and this\ntool is superseded by `git difftool --dir-diff` anyway. It seems very\nunlikely that there exists an external caller that would take into\naccount the return code of `get_merge_tool` as it would always return 0\nbefore this change so this change probably does not affect any external\nusers.\n\n[1]: https://github.com/thenigan/git-diffall\n\nSigned-off-by: Denton Liu \n---\n Documentation/git-mergetool--lib.txt | 4 +++-\n git-difftool--helper.sh | 2 +-\n git-mergetool--lib.sh | 5 ++++-\n git-mergetool.sh | 12 ++++--------\n 4 files changed, 12 insertions(+), 11 deletions(-)\n\ndiff --git a/Documentation/git-mergetool--lib.txt b/Documentation/git-mergetool--lib.txt\nindex 055550b2bc..4da9d24096 100644\n--- a/Documentation/git-mergetool--lib.txt\n+++ b/Documentation/git-mergetool--lib.txt\n@@ -28,7 +28,9 @@ to define the operation mode for the functions listed below.\n FUNCTIONS\n ---------\n get_merge_tool::\n-\treturns a merge tool.\n+\treturns a merge tool. the return code is 1 if we returned a guessed\n+\tmerge tool, else 0. '$GIT_MERGETOOL_GUI' may be set to 'true' to\n+\tsearch for the appropriate guitool.\n \n get_merge_tool_cmd::\n \treturns the custom command for a merge tool.\ndiff --git a/git-difftool--helper.sh b/git-difftool--helper.sh\nindex 7bfb6737df..46af3e60b7 100755\n--- a/git-difftool--helper.sh\n+++ b/git-difftool--helper.sh\n@@ -71,7 +71,7 @@ then\n \tthen\n \t\tmerge_tool=\"$GIT_DIFF_TOOL\"\n \telse\n-\t\tmerge_tool=\"$(get_merge_tool)\" || exit\n+\t\tmerge_tool=\"$(get_merge_tool)\"\n \tfi\n fi\n \ndiff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh\nindex 83bf52494c..b928179a2e 100644\n--- a/git-mergetool--lib.sh\n+++ b/git-mergetool--lib.sh\n@@ -403,14 +403,17 @@ get_merge_tool_path () {\n }\n \n get_merge_tool () {\n+\tis_guessed=false\n \t# Check if a merge tool has been configured\n-\tmerge_tool=$(get_configured_merge_tool)\n+\tmerge_tool=$(get_configured_merge_tool $GIT_MERGETOOL_GUI)\n \t# Try to guess an appropriate merge tool if no tool has been set.\n \tif test -z \"$merge_tool\"\n \tthen\n \t\tmerge_tool=$(guess_merge_tool) || exit\n+\t\tis_guessed=true\n \tfi\n \techo \"$merge_tool\"\n+\ttest \"$is_guessed\" = false\n }\n \n mergetool_find_win32_cmd () {\ndiff --git a/git-mergetool.sh b/git-mergetool.sh\nindex 01b9ad59b2..88fa6a914a 100755\n--- a/git-mergetool.sh\n+++ b/git-mergetool.sh\n@@ -389,7 +389,7 @@ print_noop_and_exit () {\n \n main () {\n \tprompt=$(git config --bool mergetool.prompt)\n-\tgui_tool=false\n+\tGIT_MERGETOOL_GUI=false\n \tguessed_merge_tool=false\n \torderfile=\n \n@@ -416,10 +416,10 @@ main () {\n \t\t\tesac\n \t\t\t;;\n \t\t--no-gui)\n-\t\t\tgui_tool=false\n+\t\t\tGIT_MERGETOOL_GUI=false\n \t\t\t;;\n \t\t-g|--gui)\n-\t\t\tgui_tool=true\n+\t\t\tGIT_MERGETOOL_GUI=true\n \t\t\t;;\n \t\t-y|--no-prompt)\n \t\t\tprompt=false\n@@ -449,12 +449,8 @@ main () {\n \n \tif test -z \"$merge_tool\"\n \tthen\n-\t\t# Check if a merge tool has been configured\n-\t\tmerge_tool=$(get_configured_merge_tool $gui_tool)\n-\t\t# Try to guess an appropriate merge tool if no tool has been set.\n-\t\tif test -z \"$merge_tool\"\n+\t\tif ! merge_tool=$(get_merge_tool)\n \t\tthen\n-\t\t\tmerge_tool=$(guess_merge_tool) || exit\n \t\t\tguessed_merge_tool=true\n \t\tfi\n \tfi\n-- \n2.21.0.1033.g0e8cc1100c\n\n"},{"id":"374624","messageId":"27a59e1e275f24a857435d3974f1a24fa6a475cd.1556518203.git.liu.denton@gmail.com","threadId":"50966","inReplyTo":"cover.1556518203.git.liu.denton@gmail.com","subject":"[PATCH v5 4/7] mergetool--lib: create gui_mode function","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2019-04-29T06:21:11Z","receivedAt":"2019-04-29T06:21:16Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"Before, in `get_configured_merge_tool`, we would test the value of the\nfirst argument directly, which corresponded to whether we were using\nguitool. However, since `$GIT_MERGETOOL_GUI` is available as an\nenvironment variable, create the `gui_mode` function which increases the\nclarify of functions which use it.\n\nWhile we're at it, add a space before `()` in function definitions to\nfix the style.\n\nSigned-off-by: Denton Liu \n---\n git-mergetool--lib.sh | 13 ++++++++-----\n 1 file changed, 8 insertions(+), 5 deletions(-)\n\ndiff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh\nindex b928179a2e..4ca170c8a7 100644\n--- a/git-mergetool--lib.sh\n+++ b/git-mergetool--lib.sh\n@@ -80,14 +80,18 @@ show_tool_names () {\n \t}\n }\n \n-diff_mode() {\n+diff_mode () {\n \ttest \"$TOOL_MODE\" = diff\n }\n \n-merge_mode() {\n+merge_mode () {\n \ttest \"$TOOL_MODE\" = merge\n }\n \n+gui_mode () {\n+\ttest \"$GIT_MERGETOOL_GUI\" = true\n+}\n+\n translate_merge_tool_path () {\n \techo \"$1\"\n }\n@@ -350,8 +354,7 @@ guess_merge_tool () {\n }\n \n get_configured_merge_tool () {\n-\t# If first argument is true, find the guitool instead\n-\tif test \"$1\" = true\n+\tif gui_mode\n \tthen\n \t\tgui_prefix=gui\n \tfi\n@@ -405,7 +408,7 @@ get_merge_tool_path () {\n get_merge_tool () {\n \tis_guessed=false\n \t# Check if a merge tool has been configured\n-\tmerge_tool=$(get_configured_merge_tool $GIT_MERGETOOL_GUI)\n+\tmerge_tool=$(get_configured_merge_tool)\n \t# Try to guess an appropriate merge tool if no tool has been set.\n \tif test -z \"$merge_tool\"\n \tthen\n-- \n2.21.0.1033.g0e8cc1100c\n\n"},{"id":"374625","messageId":"40413dbda11c282875811a1ddceff7542ae02aa7.1556518203.git.liu.denton@gmail.com","threadId":"50966","inReplyTo":"cover.1556518203.git.liu.denton@gmail.com","subject":"[PATCH v5 5/7] mergetool: fallback to tool when guitool unavailable","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2019-04-29T06:21:14Z","receivedAt":"2019-04-29T06:21:20Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"In git-difftool, if the tool is called with --gui but `diff.guitool` is\nnot set, it falls back to `diff.tool`. Make git-mergetool also fallback\nfrom `merge.guitool` to `merge.tool` if the former is undefined.\n\nIf git-difftool, when called with `--gui`, were to use\n`get_configured_mergetool` in a future patch, it would also get the\nfallback behavior in the following precedence:\n\n1. diff.guitool\n2. merge.guitool\n3. diff.tool\n4. merge.tool\n\nThe behavior for when difftool or mergetool are called without `--gui`\nshould be identical with or without this patch.\n\nNote that the search loop could be written as\n\n\tsections=\"merge\"\n\tkeys=\"tool\"\n\tif diff_mode\n\tthen\n\t\tsections=\"diff $sections\"\n\tfi\n\tif gui_mode\n\tthen\n\t\tkeys=\"guitool $keys\"\n\tfi\n\n\tmerge_tool=$(\n\t\tIFS=' '\n\t\tfor key in $keys\n\t\tdo\n\t\t\tfor section in $sections\n\t\t\tdo\n\t\t\t\tselected=$(git config $section.$key)\n\t\t\t\tif test -n \"$selected\"\n\t\t\t\tthen\n\t\t\t\t\techo \"$selected\"\n\t\t\t\t\treturn\n\t\t\t\tfi\n\t\t\tdone\n\t\tdone)\n\nwhich would make adding a mode in the future much easier. However,\nadding a new mode will likely never happen as it is highly discouraged\nso, as a result, it is written in its current form so that it is more\nreadable for future readers.\n\nSigned-off-by: Denton Liu \n---\n Documentation/git-mergetool.txt | 4 +++-\n git-mergetool--lib.sh | 35 ++++++++++++++++++++++++---------\n t/t7610-mergetool.sh | 19 ++++++++++++++++++\n 3 files changed, 48 insertions(+), 10 deletions(-)\n\ndiff --git a/Documentation/git-mergetool.txt b/Documentation/git-mergetool.txt\nindex 0c7975a050..6b14702e78 100644\n--- a/Documentation/git-mergetool.txt\n+++ b/Documentation/git-mergetool.txt\n@@ -83,7 +83,9 @@ success of the resolution after the custom tool has exited.\n --gui::\n \tWhen 'git-mergetool' is invoked with the `-g` or `--gui` option\n \tthe default merge tool will be read from the configured\n-\t`merge.guitool` variable instead of `merge.tool`.\n+\t`merge.guitool` variable instead of `merge.tool`. If\n+\t`merge.guitool` is not set, we will fallback to the tool\n+\tconfigured under `merge.tool`.\n \n --no-gui::\n \tThis overrides a previous `-g` or `--gui` setting and reads the\ndiff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh\nindex 4ca170c8a7..696eb49160 100644\n--- a/git-mergetool--lib.sh\n+++ b/git-mergetool--lib.sh\n@@ -354,19 +354,36 @@ guess_merge_tool () {\n }\n \n get_configured_merge_tool () {\n-\tif gui_mode\n-\tthen\n-\t\tgui_prefix=gui\n-\tfi\n-\n-\t# Diff mode first tries diff.(gui)tool and falls back to merge.(gui)tool.\n-\t# Merge mode only checks merge.(gui)tool\n+\tkeys=\n \tif diff_mode\n \tthen\n-\t\tmerge_tool=$(git config diff.${gui_prefix}tool || git config merge.${gui_prefix}tool)\n+\t\tif gui_mode\n+\t\tthen\n+\t\t\tkeys=\"diff.guitool merge.guitool diff.tool merge.tool\"\n+\t\telse\n+\t\t\tkeys=\"diff.tool merge.tool\"\n+\t\tfi\n \telse\n-\t\tmerge_tool=$(git config merge.${gui_prefix}tool)\n+\t\tif gui_mode\n+\t\tthen\n+\t\t\tkeys=\"merge.guitool merge.tool\"\n+\t\telse\n+\t\t\tkeys=\"merge.tool\"\n+\t\tfi\n \tfi\n+\n+\tmerge_tool=$(\n+\t\tIFS=' '\n+\t\tfor key in $keys\n+\t\tdo\n+\t\t\tselected=$(git config $key)\n+\t\t\tif test -n \"$selected\"\n+\t\t\tthen\n+\t\t\t\techo \"$selected\"\n+\t\t\t\treturn\n+\t\t\tfi\n+\t\tdone)\n+\n \tif test -n \"$merge_tool\" && ! valid_tool \"$merge_tool\"\n \tthen\n \t\techo >&2 \"git config option $TOOL_MODE.${gui_prefix}tool set to unknown tool: $merge_tool\"\ndiff --git a/t/t7610-mergetool.sh b/t/t7610-mergetool.sh\nindex dad607e186..5b61c10a9c 100755\n--- a/t/t7610-mergetool.sh\n+++ b/t/t7610-mergetool.sh\n@@ -167,6 +167,25 @@ test_expect_success 'gui mergetool' '\n \tgit commit -m \"branch1 resolved with mergetool\"\n '\n \n+test_expect_success 'gui mergetool without merge.guitool set falls back to merge.tool' '\n+\ttest_when_finished \"git reset --hard\" &&\n+\tgit checkout -b test$test_count branch1 &&\n+\tgit submodule update -N &&\n+\ttest_must_fail git merge master &&\n+\t( yes \"\" | git mergetool --gui both ) &&\n+\t( yes \"\" | git mergetool -g file1 file1 ) &&\n+\t( yes \"\" | git mergetool --gui file2 \"spaced name\" ) &&\n+\t( yes \"\" | git mergetool --gui subdir/file3 ) &&\n+\t( yes \"d\" | git mergetool --gui file11 ) &&\n+\t( yes \"d\" | git mergetool --gui file12 ) &&\n+\t( yes \"l\" | git mergetool --gui submod ) &&\n+\ttest \"$(cat file1)\" = \"master updated\" &&\n+\ttest \"$(cat file2)\" = \"master new\" &&\n+\ttest \"$(cat subdir/file3)\" = \"master new sub\" &&\n+\ttest \"$(cat submod/bar)\" = \"branch1 submodule\" &&\n+\tgit commit -m \"branch1 resolved with mergetool\"\n+'\n+\n test_expect_success 'mergetool crlf' '\n \ttest_when_finished \"git reset --hard\" &&\n \t# This test_config line must go after the above reset line so that\n-- \n2.21.0.1033.g0e8cc1100c\n\n"},{"id":"374626","messageId":"c70789b689937f31bcde333f3d7cbe9099e53a0e.1556518203.git.liu.denton@gmail.com","threadId":"50966","inReplyTo":"cover.1556518203.git.liu.denton@gmail.com","subject":"[PATCH v5 6/7] difftool: make --gui, --tool and --extcmd mutually exclusive","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2019-04-29T06:21:17Z","receivedAt":"2019-04-29T06:21:22Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"In git-difftool, these options specify which tool to ultimately run. As\na result, they are logically conflicting. Explicitly disallow these\noptions from being used together.\n\nSigned-off-by: Denton Liu \n---\n builtin/difftool.c | 3 +++\n t/t7800-difftool.sh | 8 ++++++++\n 2 files changed, 11 insertions(+)\n\ndiff --git a/builtin/difftool.c b/builtin/difftool.c\nindex a3ea60ea71..65bba90338 100644\n--- a/builtin/difftool.c\n+++ b/builtin/difftool.c\n@@ -731,6 +731,9 @@ int cmd_difftool(int argc, const char **argv, const char *prefix)\n \tsetenv(GIT_DIR_ENVIRONMENT, absolute_path(get_git_dir()), 1);\n \tsetenv(GIT_WORK_TREE_ENVIRONMENT, absolute_path(get_git_work_tree()), 1);\n \n+\tif (use_gui_tool + !!difftool_cmd + !!extcmd > 1)\n+\t\tdie(_(\"--gui, --tool and --extcmd are mutually exclusive\"));\n+\n \tif (use_gui_tool && diff_gui_tool && *diff_gui_tool)\n \t\tsetenv(\"GIT_DIFF_TOOL\", diff_gui_tool, 1);\n \telse if (difftool_cmd) {\ndiff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh\nindex bb9a7f4ff9..fd4a2a93b6 100755\n--- a/t/t7800-difftool.sh\n+++ b/t/t7800-difftool.sh\n@@ -705,4 +705,12 @@ test_expect_success SYMLINKS 'difftool --dir-diff handles modified symlinks' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'difftool --gui, --tool and --extcmd are mutually exclusive' '\n+\tdifftool_test_setup &&\n+\ttest_must_fail git difftool --gui --tool=test-tool &&\n+\ttest_must_fail git difftool --gui --extcmd=cat &&\n+\ttest_must_fail git difftool --tool=test-tool --extcmd=cat &&\n+\ttest_must_fail git difftool --gui --tool=test-tool --extcmd=cat\n+'\n+\n test_done\n-- \n2.21.0.1033.g0e8cc1100c\n\n"},{"id":"374627","messageId":"3fd4f46a7cd8fce71b246a60825b04c69bc1b537.1556518203.git.liu.denton@gmail.com","threadId":"50966","inReplyTo":"cover.1556518203.git.liu.denton@gmail.com","subject":"[PATCH v5 7/7] difftool: fallback on merge.guitool","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2019-04-29T06:21:20Z","receivedAt":"2019-04-29T06:21:25Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"In git-difftool.txt, it says\n\n\t'git difftool' falls back to 'git mergetool' config variables when the\n\tdifftool equivalents have not been defined.\n\nHowever, when `diff.guitool` is missing, it doesn't fallback to\nanything. Make git-difftool fallback to `merge.guitool` when `diff.guitool` is\nmissing.\n\nSigned-off-by: Denton Liu \n---\n Documentation/git-difftool.txt | 4 +++-\n builtin/difftool.c | 10 ++--------\n t/t7800-difftool.sh | 16 ++++++++++++++++\n 3 files changed, 21 insertions(+), 9 deletions(-)\n\ndiff --git a/Documentation/git-difftool.txt b/Documentation/git-difftool.txt\nindex 96c26e6aa8..484c485fd0 100644\n--- a/Documentation/git-difftool.txt\n+++ b/Documentation/git-difftool.txt\n@@ -90,7 +90,9 @@ instead. `--no-symlinks` is the default on Windows.\n \tWhen 'git-difftool' is invoked with the `-g` or `--gui` option\n \tthe default diff tool will be read from the configured\n \t`diff.guitool` variable instead of `diff.tool`. The `--no-gui`\n-\toption can be used to override this setting.\n+\toption can be used to override this setting. If `diff.guitool`\n+\tis not set, we will fallback in the order of `merge.guitool`,\n+\t`diff.tool`, `merge.tool` until a tool is found.\n \n --[no-]trust-exit-code::\n \t'git-difftool' invokes a diff tool individually on each file.\ndiff --git a/builtin/difftool.c b/builtin/difftool.c\nindex 65bba90338..10660639c0 100644\n--- a/builtin/difftool.c\n+++ b/builtin/difftool.c\n@@ -24,7 +24,6 @@\n #include \"object-store.h\"\n #include \"dir.h\"\n \n-static char *diff_gui_tool;\n static int trust_exit_code;\n \n static const char *const builtin_difftool_usage[] = {\n@@ -34,11 +33,6 @@ static const char *const builtin_difftool_usage[] = {\n \n static int difftool_config(const char *var, const char *value, void *cb)\n {\n-\tif (!strcmp(var, \"diff.guitool\")) {\n-\t\tdiff_gui_tool = xstrdup(value);\n-\t\treturn 0;\n-\t}\n-\n \tif (!strcmp(var, \"difftool.trustexitcode\")) {\n \t\ttrust_exit_code = git_config_bool(var, value);\n \t\treturn 0;\n@@ -734,8 +728,8 @@ int cmd_difftool(int argc, const char **argv, const char *prefix)\n \tif (use_gui_tool + !!difftool_cmd + !!extcmd > 1)\n \t\tdie(_(\"--gui, --tool and --extcmd are mutually exclusive\"));\n \n-\tif (use_gui_tool && diff_gui_tool && *diff_gui_tool)\n-\t\tsetenv(\"GIT_DIFF_TOOL\", diff_gui_tool, 1);\n+\tif (use_gui_tool)\n+\t\tsetenv(\"GIT_MERGETOOL_GUI\", \"true\", 1);\n \telse if (difftool_cmd) {\n \t\tif (*difftool_cmd)\n \t\t\tsetenv(\"GIT_DIFF_TOOL\", difftool_cmd, 1);\ndiff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh\nindex fd4a2a93b6..957ddf5dc6 100755\n--- a/t/t7800-difftool.sh\n+++ b/t/t7800-difftool.sh\n@@ -279,11 +279,27 @@ test_expect_success 'difftool + mergetool config variables' '\n \techo branch >expect &&\n \tgit difftool --no-prompt branch >actual &&\n \ttest_cmp expect actual &&\n+\tgit difftool --gui --no-prompt branch >actual &&\n+\ttest_cmp expect actual &&\n \n \t# set merge.tool to something bogus, diff.tool to test-tool\n \ttest_config merge.tool bogus-tool &&\n \ttest_config diff.tool test-tool &&\n \tgit difftool --no-prompt branch >actual &&\n+\ttest_cmp expect actual &&\n+\tgit difftool --gui --no-prompt branch >actual &&\n+\ttest_cmp expect actual &&\n+\n+\t# set merge.tool, diff.tool to something bogus, merge.guitool to test-tool\n+\ttest_config diff.tool bogus-tool &&\n+\ttest_config merge.guitool test-tool &&\n+\tgit difftool --gui --no-prompt branch >actual &&\n+\ttest_cmp expect actual &&\n+\n+\t# set merge.tool, diff.tool, merge.guitool to something bogus, diff.guitool to test-tool\n+\ttest_config merge.guitool bogus-tool &&\n+\ttest_config diff.guitool test-tool &&\n+\tgit difftool --gui --no-prompt branch >actual &&\n \ttest_cmp expect actual\n '\n \n-- \n2.21.0.1033.g0e8cc1100c\n\n"}]}