{"thread":{"id":"54981","subject":"[RFC PATCH] mergetools: support difftool.tabbed setting","startedAt":"2021-01-13T06:03:35Z","lastAt":"2021-02-12T22:22:30Z","messageCount":14,"participants":["Nicholas Guriev","Junio C Hamano","David Aguilar"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"414243","messageId":"2fb58fd30ae730ccd3e88ec51b5fe6d80ab7a8c7.camel@guriev.su","threadId":"54981","inReplyTo":null,"subject":"[RFC PATCH] mergetools: support difftool.tabbed setting","fromName":"Nicholas Guriev","fromEmail":"nicholas@guriev.su","sentAt":"2021-01-13T05:59:55Z","receivedAt":"2021-01-13T06:03:35Z","isPatch":true,"sender":{"key":"nicholas@guriev.su","avatar":null},"body":"I was asked how to configure \"git difftool\" to open files using several\ntabs and stop spawning diff application on every modified file. I looked\ninto Git source and found no possibility to run diff tool at one step.\n\nThe patch allows a user to view diffs in single window at one go. The\ncurrent implementation is still poor and it can be used solely for\ndemonstration purposes. To see it in action, tweak the local gitconfig:\n\n    git config difftool.prompt false\n    git config difftool.tabbed true\n\nThen run:\n\n    git difftool -t vimdiff\n\nOr:\n\n    git difftool -t meld\n\nThe solution has some restrictions, diffing up to ten files works now (I\ndid not bother with dynamic memory allocation), and it does not handle spaces\nin file names (I do not know how to pass them correctly to underlying tools\nwithout \"xargs -0\").\n\nI think the git-difftool--helper should be changed so that it could\nprocess many files in single invocation and it would not use a temporary\nfile by itself. A similar behaviour can be done in git-mergetool, too.\n\nDo you have ideas how to better implement such a feature? Any comments\nare welcome.\n\nP.S.: I'm attaching screenshots for a clear demo what I mean.\n---\n diff.c                |  4 ++--\n git-mergetool--lib.sh | 36 +++++++++++++++++++++++++++++++++++-\n mergetools/meld       |  4 ++++\n mergetools/vimdiff    | 17 +++++++++++++++++\n 4 files changed, 58 insertions(+), 3 deletions(-)\n\n\n\n\n\ndiff --git a/diff.c b/diff.c\nindex 2253ec880..8a265e0b0 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -532,7 +532,7 @@ static struct diff_tempfile {\n \t * this tempfile object is used to manage its lifetime.\n \t */\n \tstruct tempfile *tempfile;\n-} diff_temp[2];\n+} diff_temp[20];\n \n struct emit_callback {\n \tint color_diff;\n@@ -4275,7 +4275,7 @@ static void run_external_diff(const char *pgm,\n \tif (run_command_v_opt_cd_env(argv.v, RUN_USING_SHELL, NULL, env.v))\n \t\tdie(_(\"external diff died, stopping at %s\"), name);\n \n-\tremove_tempfile();\n+\t//remove_tempfile();\n \tstrvec_clear(&argv);\n \tstrvec_clear(&env);\n }\ndiff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh\nindex 7225abd81..e599e4243 100644\n--- a/git-mergetool--lib.sh\n+++ b/git-mergetool--lib.sh\n@@ -193,6 +193,8 @@ setup_tool () {\n \t\tfalse\n \t}\n \n+\tunset -f diff_combo_cmd\n+\n \tif test -f \"$MERGE_TOOLS_DIR/$tool\"\n \tthen\n \t\t. \"$MERGE_TOOLS_DIR/$tool\"\n@@ -248,6 +250,25 @@ trust_exit_code () {\n \tfi\n }\n \n+is_difftool_tabbed () {\n+\t: \"${GIT_DIFFTOOL_TABBED:=$(git config --type=bool --default=false difftool.tabbed)}\"\n+\tcase $(printf \"%s\" \"$GIT_DIFFTOOL_TABBED\" | tr '[:upper:]' '[:lower:]') in\n+\tyes|on|true|1)\n+\t\tGIT_DIFFTOOL_TABBED=true\n+\t\t;;\n+\tno|off|false|0)\n+\t\tGIT_DIFFTOOL_TABBED=false\n+\t\t;;\n+\t*)\n+\t\techo \"error: bad boolean value of GIT_DIFFTOOL_TABBED\" >&2\n+\t\texit 1\n+\t\t;;\n+\tesac\n+\n+\ttest \"$GIT_DIFFTOOL_TABBED\" = true && test \"$GIT_DIFF_PATH_TOTAL\" -gt 1 \\\n+\t\t&& type diff_combo_cmd >/dev/null 2>&1\n+}\n+\n \n # Entry point for running tools\n run_merge_tool () {\n@@ -272,7 +293,20 @@ run_merge_tool () {\n \n # Run a either a configured or built-in diff tool\n run_diff_cmd () {\n-\tdiff_cmd \"$1\"\n+\tif is_difftool_tabbed\n+\tthen\n+\t\ttemp_file=\"${TMPDIR:-/tmp}/git-${PPID}_tabbed-queue\"\n+\t\ttest \"$GIT_DIFF_PATH_COUNTER\" -eq 1 && > \"$temp_file\"\n+\t\tprintf \"%s \" \"$LOCAL\" \"$REMOTE\" >> \"$temp_file\"\n+\n+\t\tif [ \"$GIT_DIFF_PATH_COUNTER\" -eq \"$GIT_DIFF_PATH_TOTAL\" ]\n+\t\tthen\n+\t\t\tdiff_combo_cmd 3< \"$temp_file\"\n+\t\t\trm -f -- \"$temp_file\"\n+\t\tfi\n+\telse\n+\t\tdiff_cmd \"$1\"\n+\tfi\n }\n \n # Run a either a configured or built-in merge tool\ndiff --git a/mergetools/meld b/mergetools/meld\nindex aab4ebb93..6570bf0f8 100644\n--- a/mergetools/meld\n+++ b/mergetools/meld\n@@ -2,6 +2,10 @@ diff_cmd () {\n \t\"$merge_tool_path\" \"$LOCAL\" \"$REMOTE\"\n }\n \n+diff_combo_cmd () {\n+\t( IFS=' '; \"$merge_tool_path\" $(printf ' --diff %s %s' `cat <&3`) 3<&- )\n+}\n+\n merge_cmd () {\n \tcheck_meld_for_features\n \ndiff --git a/mergetools/vimdiff b/mergetools/vimdiff\nindex abc8ce4ec..31d6e1eaa 100644\n--- a/mergetools/vimdiff\n+++ b/mergetools/vimdiff\n@@ -3,6 +3,23 @@ diff_cmd () {\n \t\t-c 'wincmd l' -c 'cd $GIT_PREFIX' \"$LOCAL\" \"$REMOTE\"\n }\n \n+multitabbed_script='\n+\tlet i = 1\n+\twhile i < argc()\n+\t\texecute \"tabedit\" fnameescape(argv(i - 1))\n+\t\texecute \"diffsplit\" fnameescape(argv(i))\n+\t\twincmd L\n+\t\tlet i = i + 2\n+\tendwhile\n+\ttabfirst\n+\ttabclose\n+\tunlet i\n+'\n+diff_combo_cmd () {\n+\t( IFS=' '; cd \"$GIT_PREFIX\" && \"$merge_tool_path\" -R -f \\\n+\t\t-c \"$multitabbed_script\" `cat <&3` 3<&- )\n+}\n+\n merge_cmd () {\n \tcase \"$1\" in\n \t*vimdiff)\n"},{"id":"414623","messageId":"20210118210003.3071205-3-nicholas@guriev.su","threadId":"54981","inReplyTo":"20210118210003.3071205-1-nicholas@guriev.su","subject":"[RFC PATCH v2 2/3] difftool-helper: conciliate difftool.tabbed and difftool.prompt settings","fromName":"Nicholas Guriev","fromEmail":"nicholas@guriev.su","sentAt":"2021-01-18T21:00:02Z","receivedAt":"2021-01-18T21:04:34Z","isPatch":true,"sender":{"key":"nicholas@guriev.su","avatar":null},"body":"The commit combines paths of compared files into a separate temporary\nfile and prints them before launch the tool on last invocation of the\nhelper. All temporary files will be removed before exiting after that.\nAt least, we try. But it may happen the files remain in case of a bug\nor a random SIGKILL.\n---\n git-difftool--helper.sh | 39 ++++++++++++++++++++++++++-------------\n git-mergetool--lib.sh   | 34 +++++++++++++++++++++++++++-------\n 2 files changed, 53 insertions(+), 20 deletions(-)\n\ndiff --git a/git-difftool--helper.sh b/git-difftool--helper.sh\nindex 46af3e60b7..529d55c96d 100755\n--- a/git-difftool--helper.sh\n+++ b/git-difftool--helper.sh\n@@ -11,8 +11,8 @@ TOOL_MODE=diff\n # difftool.prompt controls the default prompt/no-prompt behavior\n # and is overridden with $GIT_DIFFTOOL*_PROMPT.\n should_prompt () {\n-\tprompt_merge=$(git config --bool mergetool.prompt || echo true)\n-\tprompt=$(git config --bool difftool.prompt || echo $prompt_merge)\n+\tprompt=$(git config --bool mergetool.prompt || echo true)\n+\tprompt=$(git config --bool difftool.prompt || echo $prompt)\n \tif test \"$prompt\" = true\n \tthen\n \t\ttest -z \"$GIT_DIFFTOOL_NO_PROMPT\"\n@@ -26,6 +26,18 @@ use_ext_cmd () {\n \ttest -n \"$GIT_DIFFTOOL_EXTCMD\"\n }\n \n+prompt_before_launch () {\n+\twhile true\n+\tdo\n+\t\tprintf \"Launch '%s' [Y/n]? \" \"${GIT_DIFFTOOL_EXTCMD:-$merge_tool}\"\n+\t\tread ans 2>/dev/null || return 1\n+\t\tcase \"${ans-y}\" in\n+\t\t[yY]*) return 0 ;;\n+\t\t[nN]*) return 1 ;;\n+\t\tesac\n+\tdone\n+}\n+\n launch_merge_tool () {\n \t# Merged is the filename as it appears in the work tree\n \t# Local is the contents of a/filename\n@@ -40,19 +52,20 @@ launch_merge_tool () {\n \t# the user with the real $MERGED name before launching $merge_tool.\n \tif should_prompt\n \tthen\n-\t\tprintf \"\\nViewing (%s/%s): '%s'\\n\" \"$GIT_DIFF_PATH_COUNTER\" \\\n-\t\t\t\"$GIT_DIFF_PATH_TOTAL\" \"$MERGED\"\n-\t\tif use_ext_cmd\n+\t\tappend_templist merged-paths \"$MERGED\"\n+\t\t# Do preinit before test whether the tool supports tabbed run.\n+\t\tuse_ext_cmd || setup_tool \"$merge_tool\"\n+\n+\t\tif ! is_difftool_tabbed\n \t\tthen\n-\t\t\tprintf \"Launch '%s' [Y/n]? \" \\\n-\t\t\t\t\"$GIT_DIFFTOOL_EXTCMD\"\n-\t\telse\n-\t\t\tprintf \"Launch '%s' [Y/n]? \" \"$merge_tool\"\n-\t\tfi\n-\t\tread ans || return\n-\t\tif test \"$ans\" = n\n+\t\t\tprintf \"\\nViewing (%d/%d): '%s'\\n\" \"$GIT_DIFF_PATH_COUNTER\" \\\n+\t\t\t\t\"$GIT_DIFF_PATH_TOTAL\" \"$MERGED\"\n+\t\t\tprompt_before_launch || return\n+\t\telif on_last_file\n \t\tthen\n-\t\t\treturn\n+\t\t\tprintf \"Viewing %d files:\\n\" \"$GIT_DIFF_PATH_TOTAL\"\n+\t\t\tprintf \"  '%s'\\n\" `cat \"$LAST_TEMPFILE\"`\n+\t\t\tprompt_before_launch || return\n \t\tfi\n \tfi\n \ndiff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh\nindex 2a1edbb9b9..4bb0e31dac 100644\n--- a/git-mergetool--lib.sh\n+++ b/git-mergetool--lib.sh\n@@ -22,6 +22,10 @@ is_available () {\n \ttype \"$merge_tool_path\" >/dev/null 2>&1\n }\n \n+on_last_file () {\n+\ttest \"$GIT_DIFF_PATH_COUNTER\" -eq \"$GIT_DIFF_PATH_TOTAL\" 2>/dev/null\n+}\n+\n list_config_tools () {\n \tsection=$1\n \tline_prefix=${2:-}\n@@ -277,6 +281,26 @@ is_difftool_tabbed () {\n \ttype diff_combo_cmd >/dev/null 2>&1\n }\n \n+# Save lines to the chosen temporary file for usage from subsequent invocations.\n+# Path to the file will be placed to the LAST_TEMPFILE variable for later links.\n+append_templist () {\n+\tLAST_TEMPFILE=\"${TMPDIR:-/tmp}/git-${PPID}_$1\"\n+\tshift\n+\n+\tif test \"$GIT_DIFF_PATH_COUNTER\" -eq 1\n+\tthen\n+\t\tprintf '%s\\n' \"$@\" >\"$LAST_TEMPFILE\"\n+\telse\n+\t\tprintf '%s\\n' \"$@\" >>\"$LAST_TEMPFILE\"\n+\tfi\n+}\n+\n+# Clean any temporary files that may create this script.\n+clean_templists () {\n+\trm -f -- \"${TMPDIR:-/tmp}/git-${PPID}\"_*\n+}\n+trap 'on_last_file && clean_templists' EXIT INT TERM\n+\n \n # Entry point for running tools\n run_merge_tool () {\n@@ -303,14 +327,10 @@ run_merge_tool () {\n run_diff_cmd () {\n \tif is_difftool_tabbed\n \tthen\n-\t\ttemp_file=\"${TMPDIR:-/tmp}/git-${PPID}_tabbed-queue\"\n-\t\ttest \"$GIT_DIFF_PATH_COUNTER\" -eq 1 && >\"$temp_file\"\n-\t\tprintf '%s\\n' \"$LOCAL\" \"$REMOTE\" >>\"$temp_file\"\n-\n-\t\tif test \"$GIT_DIFF_PATH_COUNTER\" -eq \"$GIT_DIFF_PATH_TOTAL\"\n+\t\tappend_templist tabbed-queue \"$LOCAL\" \"$REMOTE\"\n+\t\tif on_last_file\n \t\tthen\n-\t\t\tdiff_combo_cmd 3<\"$temp_file\"\n-\t\t\trm -f -- \"$temp_file\"\n+\t\t\tdiff_combo_cmd 3<\"$LAST_TEMPFILE\"\n \t\tfi\n \telse\n \t\tdiff_cmd \"$1\"\n-- \n2.27.0\n\n"},{"id":"414625","messageId":"20210118210003.3071205-2-nicholas@guriev.su","threadId":"54981","inReplyTo":"20210118210003.3071205-1-nicholas@guriev.su","subject":"[RFC PATCH v2 1/3] mergetools: support difftool.tabbed setting","fromName":"Nicholas Guriev","fromEmail":"nicholas@guriev.su","sentAt":"2021-01-18T21:00:01Z","receivedAt":"2021-01-18T21:07:57Z","isPatch":true,"sender":{"key":"nicholas@guriev.su","avatar":null},"body":"The patch introduces a new boolean setting, difftool.tabbed, which\nallows a user to view diffs in single window at one go. The `--tabbed`\ncommand line option and the GIT_DIFFTOOL_TABBED environment variable are\nalso available. For now, it works only with vimdiff and related, meld.\n\nWe call to a new invented function, diff_combo_cmd, if it is provided by\nthe diff tool, and pass to it a list of pairs of compared files. The\nlist is available through third file descriptor, filenames are separated\nby a line feed, '\\n', so they can be split by means of unmodified $IFS.\nThe function may close that descriptor after reading the list.\n---\n builtin/difftool.c    |  7 ++++++-\n diff.c                | 10 +++++++++-\n git-mergetool--lib.sh | 42 +++++++++++++++++++++++++++++++++++++++++-\n mergetools/meld       |  4 ++++\n mergetools/vimdiff    | 17 +++++++++++++++++\n 5 files changed, 77 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/difftool.c b/builtin/difftool.c\nindex 6e18e623fd..f061d3c029 100644\n--- a/builtin/difftool.c\n+++ b/builtin/difftool.c\n@@ -689,7 +689,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, no_index = 0;\n+\t    tabbed = -1, tool_help = 0, no_index = 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@@ -708,6 +708,8 @@ int cmd_difftool(int argc, const char **argv, const char *prefix)\n \t\tOPT_BOOL(0, \"tool-help\", &tool_help,\n \t\t\t N_(\"print a list of diff tools that may be used with \"\n \t\t\t    \"`--tool`\")),\n+\t\tOPT_BOOL_F(0, \"tabbed\", &tabbed,\n+\t\t\t   N_(\"use tabs in diff tool if supported\"), 0),\n \t\tOPT_BOOL(0, \"trust-exit-code\", &trust_exit_code,\n \t\t\t N_(\"make 'git-difftool' exit when an invoked diff \"\n \t\t\t    \"tool returns a non - zero exit code\")),\n@@ -756,6 +758,9 @@ int cmd_difftool(int argc, const char **argv, const char *prefix)\n \t\t\tdie(_(\"no <cmd> given for --extcmd=<cmd>\"));\n \t}\n \n+\tif (tabbed >= 0)\n+\t\tsetenv(\"GIT_DIFFTOOL_TABBED\", tabbed ? \"true\" : \"false\", 1);\n+\n \tsetenv(\"GIT_DIFFTOOL_TRUST_EXIT_CODE\",\n \t       trust_exit_code ? \"true\" : \"false\", 1);\n \ndiff --git a/diff.c b/diff.c\nindex 2253ec8802..2868848bba 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -1730,6 +1730,14 @@ static struct diff_tempfile *claim_diff_tempfile(void)\n \tBUG(\"diff is failing to clean up its tempfiles\");\n }\n \n+static void forget_tempfile(void)\n+{\n+\tfor (int i = 0; i < ARRAY_SIZE(diff_temp); i++) {\n+\t\tclose_tempfile_gently(diff_temp[i].tempfile);\n+\t\tdiff_temp[i] = (struct diff_tempfile){0};\n+\t}\n+}\n+\n static void remove_tempfile(void)\n {\n \tint i;\n@@ -4275,7 +4283,7 @@ static void run_external_diff(const char *pgm,\n \tif (run_command_v_opt_cd_env(argv.v, RUN_USING_SHELL, NULL, env.v))\n \t\tdie(_(\"external diff died, stopping at %s\"), name);\n \n-\tremove_tempfile();\n+\tforget_tempfile();\n \tstrvec_clear(&argv);\n \tstrvec_clear(&env);\n }\ndiff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh\nindex 78f3647ed9..2a1edbb9b9 100644\n--- a/git-mergetool--lib.sh\n+++ b/git-mergetool--lib.sh\n@@ -195,6 +195,11 @@ setup_tool () {\n \t\tfalse\n \t}\n \n+\t# Clear combo function declared by a previous tool (if any) to\n+\t# unambiguously indicate that the current one supports the feature or\n+\t# not.\n+\tunset -f diff_combo_cmd\n+\n \tif test -f \"$MERGE_TOOLS_DIR/$tool\"\n \tthen\n \t\t. \"$MERGE_TOOLS_DIR/$tool\"\n@@ -250,6 +255,28 @@ trust_exit_code () {\n \tfi\n }\n \n+# Check whether tabbed mode is requested and current loaded tool supports it.\n+is_difftool_tabbed () {\n+\t: \"${GIT_DIFFTOOL_TABBED=$(git config --type=bool \\\n+\t\t--default=false difftool.tabbed || echo bad value)}\"\n+\tcase $(printf \"%s\" \"$GIT_DIFFTOOL_TABBED\" | tr '[:upper:]' '[:lower:]') in\n+\tyes|on|true|1)\n+\t\tGIT_DIFFTOOL_TABBED=true\n+\t\t;;\n+\tno|off|false|0|'')\n+\t\tGIT_DIFFTOOL_TABBED=false\n+\t\t;;\n+\t*)\n+\t\techo \"error: bad boolean value of GIT_DIFFTOOL_TABBED\" >&2\n+\t\texit 1\n+\t\t;;\n+\tesac\n+\n+\ttest \"$GIT_DIFFTOOL_TABBED\" = true &&\n+\ttest \"${GIT_DIFF_PATH_TOTAL=0}\" -gt 1 &&\n+\ttype diff_combo_cmd >/dev/null 2>&1\n+}\n+\n \n # Entry point for running tools\n run_merge_tool () {\n@@ -274,7 +301,20 @@ run_merge_tool () {\n \n # Run a either a configured or built-in diff tool\n run_diff_cmd () {\n-\tdiff_cmd \"$1\"\n+\tif is_difftool_tabbed\n+\tthen\n+\t\ttemp_file=\"${TMPDIR:-/tmp}/git-${PPID}_tabbed-queue\"\n+\t\ttest \"$GIT_DIFF_PATH_COUNTER\" -eq 1 && >\"$temp_file\"\n+\t\tprintf '%s\\n' \"$LOCAL\" \"$REMOTE\" >>\"$temp_file\"\n+\n+\t\tif test \"$GIT_DIFF_PATH_COUNTER\" -eq \"$GIT_DIFF_PATH_TOTAL\"\n+\t\tthen\n+\t\t\tdiff_combo_cmd 3<\"$temp_file\"\n+\t\t\trm -f -- \"$temp_file\"\n+\t\tfi\n+\telse\n+\t\tdiff_cmd \"$1\"\n+\tfi\n }\n \n # Run a either a configured or built-in merge tool\ndiff --git a/mergetools/meld b/mergetools/meld\nindex aab4ebb935..2f40262a70 100644\n--- a/mergetools/meld\n+++ b/mergetools/meld\n@@ -2,6 +2,10 @@ diff_cmd () {\n \t\"$merge_tool_path\" \"$LOCAL\" \"$REMOTE\"\n }\n \n+diff_combo_cmd () {\n+\t\"$merge_tool_path\" $(printf -- '--diff\\n%s\\n%s\\n' `cat <&3`) 3<&-\n+}\n+\n merge_cmd () {\n \tcheck_meld_for_features\n \ndiff --git a/mergetools/vimdiff b/mergetools/vimdiff\nindex abc8ce4ec4..006e7b825d 100644\n--- a/mergetools/vimdiff\n+++ b/mergetools/vimdiff\n@@ -3,6 +3,23 @@ diff_cmd () {\n \t\t-c 'wincmd l' -c 'cd $GIT_PREFIX' \"$LOCAL\" \"$REMOTE\"\n }\n \n+multitabbed_script='\n+\tlet i = 1\n+\twhile i < argc()\n+\t\texecute \"tabedit\" fnameescape(argv(i - 1))\n+\t\texecute \"diffsplit\" fnameescape(argv(i))\n+\t\twincmd L\n+\t\tlet i = i + 2\n+\tendwhile\n+\ttabfirst\n+\ttabclose\n+\tunlet i\n+\tchdir $GIT_PREFIX\n+'\n+diff_combo_cmd () {\n+\t\"$merge_tool_path\" -R -f -c \"$multitabbed_script\" -- `cat <&3` 3<&-\n+}\n+\n merge_cmd () {\n \tcase \"$1\" in\n \t*vimdiff)\n-- \n2.27.0\n\n"},{"id":"414627","messageId":"20210118210003.3071205-4-nicholas@guriev.su","threadId":"54981","inReplyTo":"20210118210003.3071205-1-nicholas@guriev.su","subject":"[RFC PATCH v2 3/3] doc: describe new difftool.tabbed feature","fromName":"Nicholas Guriev","fromEmail":"nicholas@guriev.su","sentAt":"2021-01-18T21:00:03Z","receivedAt":"2021-01-18T21:08:25Z","isPatch":true,"sender":{"key":"nicholas@guriev.su","avatar":null},"body":"And related the `--tabbed` command line option and the\nGIT_DIFFTOOL_TABBED environment variable.\n---\n Documentation/config/difftool.txt |  6 ++++++\n Documentation/git-difftool.txt    | 19 +++++++++++++++----\n Documentation/git.txt             |  4 ++++\n 3 files changed, 25 insertions(+), 4 deletions(-)\n\ndiff --git a/Documentation/config/difftool.txt b/Documentation/config/difftool.txt\nindex 6762594480..ac609aee66 100644\n--- a/Documentation/config/difftool.txt\n+++ b/Documentation/config/difftool.txt\n@@ -12,3 +12,9 @@ difftool.<tool>.cmd::\n \n difftool.prompt::\n \tPrompt before each invocation of the diff tool.\n+\n+difftool.tabbed::\n+\tShow compared files in different tabs using single invocation of\n+\tthe diff tool. Must be a boolean value. Only the following tools\n+\tare currently supported: vimdiff and related, meld. Tools with\n+\toverridden command line will ignore this configuration variable.\ndiff --git a/Documentation/git-difftool.txt b/Documentation/git-difftool.txt\nindex 484c485fd0..1b7a5345ad 100644\n--- a/Documentation/git-difftool.txt\n+++ b/Documentation/git-difftool.txt\n@@ -69,6 +69,13 @@ with custom merge tool commands and has the same value as `$MERGED`.\n --tool-help::\n \tPrint a list of diff tools that may be used with `--tool`.\n \n+--[no-]tabbed::\n+\tOpen compared files in different tabs using single invocation\n+\tof the diff tool. This overrides configuration or environment.\n+\tCurrently, only the following tools are supported: vimdiff and\n+\trelated, meld. Tools with overridden command line will ignore\n+\tthis option.\n+\n --[no-]symlinks::\n \t'git difftool''s default behavior is create symlinks to the\n \tworking tree when run in `--dir-diff` mode and the right-hand\n@@ -95,10 +102,11 @@ instead.  `--no-symlinks` is the default on Windows.\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.\n-\tErrors reported by the diff tool are ignored by default.\n-\tUse `--trust-exit-code` to make 'git-difftool' exit when an\n-\tinvoked diff tool returns a non-zero exit code.\n+\t'git-difftool' invokes a diff tool individually on each file\n+\tunless tabbed mode is active. Errors reported by the diff tool\n+\tare ignored by default. Use `--trust-exit-code` to make\n+\t'git-difftool' exit immediately when an invoked diff tool\n+\treturns a non-zero exit code.\n +\n 'git-difftool' will forward the exit code of the invoked tool when\n `--trust-exit-code` is used.\n@@ -128,6 +136,9 @@ See the `--tool=<tool>` option above for more details.\n difftool.prompt::\n \tPrompt before each invocation of the diff tool.\n \n+difftool.tabbed::\n+\tConfigure default value of the `--tabbed` option. See above.\n+\n difftool.trustExitCode::\n \tExit difftool if the invoked diff tool returns a non-zero exit status.\n +\ndiff --git a/Documentation/git.txt b/Documentation/git.txt\nindex a6d4ad0818..d7cd6650c9 100644\n--- a/Documentation/git.txt\n+++ b/Documentation/git.txt\n@@ -583,6 +583,10 @@ For each path `GIT_EXTERNAL_DIFF` is called, two environment variables,\n `GIT_DIFF_PATH_TOTAL`::\n \tThe total number of paths.\n \n+`GIT_DIFFTOOL_TABBED`::\n+\tRun the diff tool in tabbed mode opening all compared files\n+\ttogether. It must contain a boolean value.\n+\n other\n ~~~~~\n `GIT_MERGE_VERBOSITY`::\n-- \n2.27.0\n\n"},{"id":"414628","messageId":"20210118210003.3071205-1-nicholas@guriev.su","threadId":"54981","inReplyTo":"2fb58fd30ae730ccd3e88ec51b5fe6d80ab7a8c7.camel@guriev.su","subject":"[RFC PATCH v2 0/3] implement tabbed mode in difftools","fromName":"Nicholas Guriev","fromEmail":"nicholas@guriev.su","sentAt":"2021-01-18T21:00:00Z","receivedAt":"2021-01-18T21:08:26Z","isPatch":true,"sender":{"key":"nicholas@guriev.su","avatar":null},"body":"In this mode the \"git difftool\" command opens all compared files via single\ninvocation of an editor passing to it entire list of the changed files. I find\nit useful, and it allows a user to switch easily forward and back between the\nfiles. You will find some screenshots demonstrating the feature in my previous\nmessage.\n\nThe patch series looks better now, and the two problems mentioned earlier have\nbeen solved. I added a new static function, forget_tempfile, into the diff.c\nfile. It cleans diff_temp structures and do not remove the temporary files.\nThey anyway would be deleted in an atexit handler. In this way, I can collect\ncontent of all changed files before running the editor.\n\nFor solving the second problem with spaces, I am separating file names with\nline-feeds, '\\n', that are less common. This restriction is the same with the\n\"git mergetool\" command. I think it is acceptable at the present stage.\n\nI have also repaired prompting in the tabbed mode. In such case, the command\nasks a user only once right before starting the editor. And I have described\nbriefly the new changes to the best of my ability.\n\nAlas, some other problems still remain. First, some automated tests failed that\nrelated to the difftool.prompt setting. I have modified its behavior a bit and\nit now contains a loop. Second, there are no new tests, yet I am going to add\nthem later. And third, the main remaining problem is the method of calling of\nthe difftool helper. I do not like that I have to write temporary files in the\nhelper code to store its state between invocations. But maybe, someone offer a\nbetter solution that is easy to incorporate into the current architecture.\n\nIn conclusion, the patches are still not ready for merging, and any comments or\ntesting are welcome.\n\n\nNicholas Guriev (3):\n  mergetools: support difftool.tabbed setting\n  difftool-helper: conciliate difftool.tabbed and difftool.prompt\n    settings\n  doc: describe new difftool.tabbed feature\n\n Documentation/config/difftool.txt |  6 +++\n Documentation/git-difftool.txt    | 19 ++++++++--\n Documentation/git.txt             |  4 ++\n builtin/difftool.c                |  7 +++-\n diff.c                            | 10 ++++-\n git-difftool--helper.sh           | 39 ++++++++++++-------\n git-mergetool--lib.sh             | 62 ++++++++++++++++++++++++++++++-\n mergetools/meld                   |  4 ++\n mergetools/vimdiff                | 17 +++++++++\n 9 files changed, 148 insertions(+), 20 deletions(-)\n\n-- \n2.27.0\n\n"},{"id":"414637","messageId":"xmqqv9btj09z.fsf@gitster.c.googlers.com","threadId":"54981","inReplyTo":"20210118210003.3071205-2-nicholas@guriev.su","subject":"Re: [RFC PATCH v2 1/3] mergetools: support difftool.tabbed setting","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-01-18T23:25:12Z","receivedAt":"2021-01-18T23:26:16Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nicholas Guriev <nicholas@guriev.su> writes:\n\n> diff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh\n> index 78f3647ed9..2a1edbb9b9 100644\n> --- a/git-mergetool--lib.sh\n> +++ b/git-mergetool--lib.sh\n> @@ -195,6 +195,11 @@ setup_tool () {\n>  \t\tfalse\n>  \t}\n>  \n> +\t# Clear combo function declared by a previous tool (if any) to\n> +\t# unambiguously indicate that the current one supports the feature or\n> +\t# not.\n> +\tunset -f diff_combo_cmd\n> +\n\nI know this is so that you can use \"type diff_combo_cmd\" in a\ndifferent part of the patch, but I do not think you want to be\nfooled by an unrelated ~/bin/diff_combo_cmd that the user may have\nunder the home directory.\n\nInstead, the fallback definitions can have\n\n\tdiff_combo_supported () {\n\t\treturn 1\n\t}\n\nand have backends that does support diff_combo_supported to override\nwith their own\n\n\tdiff_combo_supported () {\n\t\treturn 0\n\t}\n\nAnd then, the part of is_difftool_tabbed that wants to see if the\ncurrent backend supports the operation can become:\n\n\ttest \"$GIT_DIFFTOOL_TABBED\" = true &&\n\ttest \"${GIT_DIFF_PATH_TOTAL=0}\" -gt 1 &&\n-\ttype diff_combo_cmd >/dev/null 2>&1\n+\tdiff_combo_supported\n }\n\nThat way, you do not have to do \"unset -f\" up above.\n\n\n> @@ -250,6 +255,28 @@ trust_exit_code () {\n>  \tfi\n>  }\n>  \n> +# Check whether tabbed mode is requested and current loaded tool supports it.\n> +is_difftool_tabbed () {\n> +\t: \"${GIT_DIFFTOOL_TABBED=$(git config --type=bool \\\n> +\t\t--default=false difftool.tabbed || echo bad value)}\"\n> +\tcase $(printf \"%s\" \"$GIT_DIFFTOOL_TABBED\" | tr '[:upper:]' '[:lower:]') in\n> +\tyes|on|true|1)\n> +\t\tGIT_DIFFTOOL_TABBED=true\n> +\t\t;;\n> +\tno|off|false|0|'')\n> +\t\tGIT_DIFFTOOL_TABBED=false\n> +\t\t;;\n> +\t*)\n> +\t\techo \"error: bad boolean value of GIT_DIFFTOOL_TABBED\" >&2\n> +\t\texit 1\n> +\t\t;;\n> +\tesac\n> +\n> +\ttest \"$GIT_DIFFTOOL_TABBED\" = true &&\n> +\ttest \"${GIT_DIFF_PATH_TOTAL=0}\" -gt 1 &&\n> +\ttype diff_combo_cmd >/dev/null 2>&1\n> +}\n"},{"id":"415237","messageId":"20210125212132.894458-5-nicholas@guriev.su","threadId":"54981","inReplyTo":"20210125212132.894458-1-nicholas@guriev.su","subject":"[PATCH v3 4/4] t7800: new tests of difftool.tabbed feature","fromName":"Nicholas Guriev","fromEmail":"nicholas@guriev.su","sentAt":"2021-01-25T21:21:32Z","receivedAt":"2021-01-25T21:23:22Z","isPatch":true,"sender":{"key":"nicholas@guriev.su","avatar":null},"body":"A few helper functions were put in the script that manage faked binaries in a\ntemporary directory for $PATH. The helpers restore state before test finish.\n\nBesides, two tests of rewritten prompting were added. \"git difftool\" now asks\nagain on incorrect input. Plus, fixed a typo in a test of the --extcmd option.\n\nSigned-off-by: Nicholas Guriev <nicholas@guriev.su>\n---\n t/t7800-difftool.sh | 175 +++++++++++++++++++++++++++++++++++++++++++-\n 1 file changed, 174 insertions(+), 1 deletion(-)\n\ndiff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh\nindex a578b35761..249d7a4821 100755\n--- a/t/t7800-difftool.sh\n+++ b/t/t7800-difftool.sh\n@@ -270,6 +270,22 @@ test_expect_success 'difftool last flag wins' '\n \tprompt_given \"$prompt\"\n '\n \n+test_expect_success 'ignore unknown input then launch the tool' '\n+\tdifftool_test_setup &&\n+\t(echo Qwerty && echo Yes) >input &&\n+\tgit difftool branch <input >output &&\n+\ttail -1 output | grep -q -x -F \\\n+\t\t\"Launch '\\''test-tool'\\'' [Y/n]? Launch '\\''test-tool'\\'' [Y/n]? branch\"\n+'\n+\n+test_expect_success 'ignore unknown input then skip the tool' '\n+\tdifftool_test_setup &&\n+\t(echo Qwerty && echo No) >input &&\n+\tgit difftool branch <input >output &&\n+\ttail -1 output | grep -q -x -F \\\n+\t\t\"Launch '\\''test-tool'\\'' [Y/n]? Launch '\\''test-tool'\\'' [Y/n]? \"\n+'\n+\n # git-difftool falls back to git-mergetool config variables\n # so test that behavior here\n test_expect_success 'difftool + mergetool config variables' '\n@@ -319,7 +335,7 @@ test_expect_success 'difftool --extcmd=cat' '\n test_expect_success 'difftool --extcmd cat' '\n \techo branch >expect &&\n \techo master >>expect &&\n-\tgit difftool --no-prompt --extcmd=cat branch >actual &&\n+\tgit difftool --no-prompt --extcmd cat branch >actual &&\n \ttest_cmp expect actual\n '\n \n@@ -390,6 +406,163 @@ test_expect_success 'ending prompt input with EOF' '\n \t! grep br2 output\n '\n \n+prepend_shared_path () {\n+\tdirectory=\"$1\"\n+\tfirst=\"${PATH%%:*}\"\n+\tif test \"$directory\" != \"$first\"\n+\tthen\n+\t\t: ${clean_shared_path=$PATH}\n+\t\tPATH=\"$directory:$PATH\"\n+\t\ttest_when_finished 'PATH=$clean_shared_path'\n+\tfi\n+}\n+\n+mock_binary () {\n+\tname=\"$1\" body=\"$2\"\n+\tprepend_shared_path \"$PWD/fake-bin\" &&\n+\ttouch run-cmds &&\n+\tmkdir -p fake-bin &&\n+\twrite_script \"fake-bin/$name\" <<-EOF &&\n+\tprintf '%s ' $name \"\\$@\" |\n+\ttr -s '[:space:]' ' ' |\n+\tsed 's/ \\$/\\\\n/' >>\"\\$HOME/run-cmds\" &&\n+\t${body:-true}\n+\tEOF\n+\ttest_when_finished \"rm -f fake-bin/$name run-cmds\"\n+}\n+\n+test_invoked () {\n+\tpattern=\"$1\"\n+\tif ! grep -q -x -e \"$pattern\" run-cmds\n+\tthen\n+\t\tset -- $pattern\n+\t\techo \"$1 does not seem to be invoked\"\n+\t\techo \"entry '$pattern' was not found in the 'run-cmds' file\"\n+\t\tfalse\n+\tfi\n+}\n+\n+test_expect_success 'tabbed mode with --tabbed option' '\n+\tmock_binary vim &&\n+\ttest_config_global diff.tool vimdiff &&\n+\tyes | git difftool --tabbed branch | tee actual &&\n+\t(\n+\t\tprintf \"Viewing 2 files:\\n\" &&\n+\t\tprintf \"  '\\''file'\\''\\n\" &&\n+\t\tprintf \"  '\\''file2'\\''\\n\" &&\n+\t\tprintf \"Launch '\\''vimdiff'\\'' [Y/n]? \"\n+\t) >expect &&\n+\ttest_cmp expect actual &&\n+\ttest_invoked \"vim -R -f -c .* file .* file2\"\n+'\n+\n+test_expect_success 'tabbed mode with GIT_DIFFTOOL_TABBED environment' '\n+\tmock_binary vim &&\n+\ttest_config_global diff.tool vimdiff &&\n+\tyes | GIT_DIFFTOOL_TABBED=yes git difftool branch | tee actual &&\n+\t(\n+\t\tprintf \"Viewing 2 files:\\n\" &&\n+\t\tprintf \"  '\\''file'\\''\\n\" &&\n+\t\tprintf \"  '\\''file2'\\''\\n\" &&\n+\t\tprintf \"Launch '\\''vimdiff'\\'' [Y/n]? \"\n+\t) >expect &&\n+\ttest_cmp expect actual &&\n+\ttest_invoked \"vim -R -f -c .* file .* file2\"\n+'\n+\n+test_expect_success 'tabbed mode with difftool.tabbed setting' '\n+\tmock_binary vim &&\n+\ttest_config_global diff.tool vimdiff &&\n+\ttest_config_global difftool.tabbed 1 &&\n+\tyes | git difftool branch | tee actual &&\n+\t(\n+\t\tprintf \"Viewing 2 files:\\n\" &&\n+\t\tprintf \"  '\\''file'\\''\\n\" &&\n+\t\tprintf \"  '\\''file2'\\''\\n\" &&\n+\t\tprintf \"Launch '\\''vimdiff'\\'' [Y/n]? \"\n+\t) >expect &&\n+\ttest_cmp expect actual &&\n+\ttest_invoked \"vim -R -f -c .* file .* file2\"\n+'\n+\n+test_expect_success 'environment variable wins over config in tabbed mode' '\n+\tmock_binary vim &&\n+\ttest_config_global diff.tool vimdiff &&\n+\ttest_config_global difftool.tabbed true &&\n+\tGIT_DIFFTOOL_TABBED=FALSE git difftool -y branch </dev/null >output &&\n+\ttest_must_be_empty output &&\n+\ttest_invoked \"vim -R -f -d -c .* file\" &&\n+\ttest_invoked \"vim -R -f -d -c .* file2\"\n+'\n+\n+test_expect_success 'cli option wins over environment in tabbed mode' '\n+\tmock_binary vim &&\n+\ttest_config_global diff.tool vimdiff &&\n+\tGIT_DIFFTOOL_TABBED=1 git difftool -y --no-tabbed branch </dev/null >output &&\n+\ttest_must_be_empty output &&\n+\ttest_invoked \"vim -R -f -d -c .* file\" &&\n+\ttest_invoked \"vim -R -f -d -c .* file2\"\n+'\n+\n+test_expect_success 'say no in tabbed mode' '\n+\tmock_binary meld &&\n+\tyes no | git difftool -t meld --tabbed branch &&\n+\t! test_invoked \"meld\\\\>.*\"\n+'\n+\n+test_expect_success 'no tabbed mode for single file' '\n+\tmock_binary meld &&\n+\tgit difftool -y -t meld --tabbed branch file &&\n+\ttest_invoked \"meld \\\\S\\\\+ file\"\n+'\n+\n+test_expect_success 'both --tabbed and --trust-exit-code options' '\n+\tmock_binary meld false &&\n+\ttest_config_global diff.tool meld &&\n+\ttest_config_global difftool.prompt false &&\n+\ttest_must_fail git difftool --tabbed --trust-exit-code branch >output &&\n+\ttest_must_be_empty output &&\n+\ttest_invoked \"meld --diff \\\\S\\\\+ file --diff \\\\S\\\\+ file2\"\n+'\n+\n+test_expect_success 'tempdir is still clean after successful tabbed mode' '\n+\tmock_binary meld &&\n+\tmkdir tempdir &&\n+\ttest_when_finished \"rm -r tempdir\" &&\n+\tTMPDIR=\"$PWD/tempdir\" git difftool -y -t meld --tabbed branch &&\n+\ttest_dir_is_empty tempdir\n+'\n+\n+test_expect_success 'tempdir is still clean after failed tabbed mode' '\n+\tmock_binary meld false &&\n+\tmkdir tempdir &&\n+\ttest_when_finished \"rm -r tempdir\" &&\n+\tTMPDIR=\"$PWD/tempdir\" git difftool -y -t meld --tabbed branch &&\n+\ttest_dir_is_empty tempdir\n+'\n+\n+test_background () {\n+\t# https://stackoverflow.com/a/45112755/5000805\n+\tset -m\n+\t\"$@\" &\n+\tset +m\n+}\n+\n+# Create a named queue for synchronizing. Our test process will get blocked on\n+# \"echo line\" until faked meld reaches \"cat chain\", and so we do not kill early.\n+test_expect_success PIPE 'tempdir is still clean after SIGTERM in tabbed mode' '\n+\tmkfifo chan &&\n+\tmock_binary meld \"while true; do cat chan; done\" &&\n+\tmkdir tempdir &&\n+\ttest_when_finished \"rm -r tempdir\" &&\n+\ttest_background env TMPDIR=\"$PWD/tempdir\" \\\n+\t\tgit difftool -y -t meld --tabbed branch &&\n+\techo line >chan &&\n+\tkill -TERM -$! &&\n+\twait && # for clean up\n+\ttest_dir_is_empty tempdir\n+'\n+\n test_expect_success 'difftool --tool-help' '\n \tgit difftool --tool-help >output &&\n \tgrep tool output\n-- \n2.27.0\n\n"},{"id":"415238","messageId":"20210125212132.894458-4-nicholas@guriev.su","threadId":"54981","inReplyTo":"20210125212132.894458-1-nicholas@guriev.su","subject":"[PATCH v3 3/4] doc: describe new difftool.tabbed feature","fromName":"Nicholas Guriev","fromEmail":"nicholas@guriev.su","sentAt":"2021-01-25T21:21:31Z","receivedAt":"2021-01-25T21:24:03Z","isPatch":true,"sender":{"key":"nicholas@guriev.su","avatar":null},"body":"And related the `--tabbed` command line option, and the\nGIT_DIFFTOOL_TABBED environment variable.\n\nSigned-off-by: Nicholas Guriev <nicholas@guriev.su>\n---\n Documentation/config/difftool.txt    |  6 +++\n Documentation/git-difftool.txt       | 19 +++++++--\n Documentation/git-mergetool--lib.txt | 62 ++++++++++++++++++++++++++++\n Documentation/git.txt                |  8 ++++\n 4 files changed, 91 insertions(+), 4 deletions(-)\n\ndiff --git a/Documentation/config/difftool.txt b/Documentation/config/difftool.txt\nindex 6762594480..ac609aee66 100644\n--- a/Documentation/config/difftool.txt\n+++ b/Documentation/config/difftool.txt\n@@ -12,3 +12,9 @@ difftool.<tool>.cmd::\n \n difftool.prompt::\n \tPrompt before each invocation of the diff tool.\n+\n+difftool.tabbed::\n+\tShow compared files in different tabs using single invocation of\n+\tthe diff tool. Must be a boolean value. Only the following tools\n+\tare currently supported: vimdiff and related, meld. Tools with\n+\toverridden command line will ignore this configuration variable.\ndiff --git a/Documentation/git-difftool.txt b/Documentation/git-difftool.txt\nindex 484c485fd0..1b7a5345ad 100644\n--- a/Documentation/git-difftool.txt\n+++ b/Documentation/git-difftool.txt\n@@ -69,6 +69,13 @@ with custom merge tool commands and has the same value as `$MERGED`.\n --tool-help::\n \tPrint a list of diff tools that may be used with `--tool`.\n \n+--[no-]tabbed::\n+\tOpen compared files in different tabs using single invocation\n+\tof the diff tool. This overrides configuration or environment.\n+\tCurrently, only the following tools are supported: vimdiff and\n+\trelated, meld. Tools with overridden command line will ignore\n+\tthis option.\n+\n --[no-]symlinks::\n \t'git difftool''s default behavior is create symlinks to the\n \tworking tree when run in `--dir-diff` mode and the right-hand\n@@ -95,10 +102,11 @@ instead.  `--no-symlinks` is the default on Windows.\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.\n-\tErrors reported by the diff tool are ignored by default.\n-\tUse `--trust-exit-code` to make 'git-difftool' exit when an\n-\tinvoked diff tool returns a non-zero exit code.\n+\t'git-difftool' invokes a diff tool individually on each file\n+\tunless tabbed mode is active. Errors reported by the diff tool\n+\tare ignored by default. Use `--trust-exit-code` to make\n+\t'git-difftool' exit immediately when an invoked diff tool\n+\treturns a non-zero exit code.\n +\n 'git-difftool' will forward the exit code of the invoked tool when\n `--trust-exit-code` is used.\n@@ -128,6 +136,9 @@ See the `--tool=<tool>` option above for more details.\n difftool.prompt::\n \tPrompt before each invocation of the diff tool.\n \n+difftool.tabbed::\n+\tConfigure default value of the `--tabbed` option. See above.\n+\n difftool.trustExitCode::\n \tExit difftool if the invoked diff tool returns a non-zero exit status.\n +\ndiff --git a/Documentation/git-mergetool--lib.txt b/Documentation/git-mergetool--lib.txt\nindex 4da9d24096..1b9fb3591e 100644\n--- a/Documentation/git-mergetool--lib.txt\n+++ b/Documentation/git-mergetool--lib.txt\n@@ -44,6 +44,68 @@ run_merge_tool::\n \t'$MERGED', '$LOCAL', '$REMOTE', and '$BASE' must be defined\n \tfor use by the merge tool.\n \n+TOOLS\n+-----\n+\n+There are several built-in merge tool wrappers which are located in the\n+'$(git --exec-path)/mergetools' directory. They are shell scripts and provide\n+a unified interface for the discussed scriptlet. It expects to find the\n+following functions defined by the each wrapper. However, most of them have\n+sane default implementation and the wrapper may write less boilerplate.\n+\n+can_merge::\n+\treturns zero status (true) if the tool can be used by `git mergetool`,\n+\totherwise the command will be unavailable. Default: true.\n+\n+can_diff::\n+\treturns zero status (true) if the tool can be used by `git difftool`,\n+\totherwise the command will be unavailable. Default: true.\n+\n+merge_cmd::\n+\tshould actually launch the tool in merging mode for a single path.\n+\tPositional argument: '$1' -- name of the merge tool. Predefined\n+\tvariables: '$MERGED', '$LOCAL', '$REMOTE', and '$BASE' mentioned above,\n+\t'$merge_tool_path' -- absolute path to the binary of the tool or its name\n+\tif seen in default search path, '$base_present' -- string `true` or `false`.\n+\n+diff_cmd::\n+\tshould actually launch the tool in diffing mode for a single path.\n+\tPositional argument: '$1' -- name of the diff tool. Predefined\n+\tvariables: '$LOCAL' and '$REMOTE', '$merge_tool_path' -- absolute path\n+\tto the binary of the tool or its name if seen in default search path.\n+\n+diff_combo_supported::\n+\treturns zero status (true) if the tool can operate in tabbed mode,\n+\totherwise the feature will be skipped. Default: false.\n+\n+diff_combo_cmd::\n+\tshould actually launch the tool with all compared files. The function\n+\treceives list of pairs of files to compare, every filename is followed\n+\tby proper input field separator, '$IFS', the list is reachable through\n+\tthird opened file descriptor and the function may close this fd after\n+\treading all its content. System file descriptors remain untouched.\n+\tPredefined variable: '$merge_tool_path' -- absolute path to the binary\n+\tof the tool or its name if seen in default search path.\n++\n+Example:\n++\n+[listing]\n+diff_combo_cmd () {\n+\t\"$merge_tool_path\" -- `cat <&3` 3<&-\n+}\n+\n+translate_merge_tool_path::\n+\tcan print preset '$merge_tool_path'. Positional argument: '$1' -- name\n+\tof the merge tool.\n+\n+exit_code_trustable::\n+\treturns zero status (true) if a caller is allowed to rely on the exit\n+\tcode of the merge tool, otherwise one should check a backup of the file\n+\tto determine whether or not merging was successful. Default: false.\n+\n+list_tool_variants::\n+\tprints all possible names that may be passed to the '--tool' option.\n+\n GIT\n ---\n Part of the linkgit:git[1] suite\ndiff --git a/Documentation/git.txt b/Documentation/git.txt\nindex a6d4ad0818..3ef75aae36 100644\n--- a/Documentation/git.txt\n+++ b/Documentation/git.txt\n@@ -583,6 +583,14 @@ For each path `GIT_EXTERNAL_DIFF` is called, two environment variables,\n `GIT_DIFF_PATH_TOTAL`::\n \tThe total number of paths.\n \n+`GIT_DIFFTOOL_PROMPT`::\n+\tIssue an interactive prompting right before launch the diff\n+\ttool. It must contain a boolean value.\n+\n+`GIT_DIFFTOOL_TABBED`::\n+\tRun the diff tool in tabbed mode opening all compared files\n+\ttogether. It must contain a boolean value.\n+\n other\n ~~~~~\n `GIT_MERGE_VERBOSITY`::\n-- \n2.27.0\n\n"},{"id":"415239","messageId":"20210125212132.894458-2-nicholas@guriev.su","threadId":"54981","inReplyTo":"20210125212132.894458-1-nicholas@guriev.su","subject":"[PATCH v3 1/4] mergetools: support difftool.tabbed setting","fromName":"Nicholas Guriev","fromEmail":"nicholas@guriev.su","sentAt":"2021-01-25T21:21:29Z","receivedAt":"2021-01-25T21:24:36Z","isPatch":true,"sender":{"key":"nicholas@guriev.su","avatar":null},"body":"The patch introduces a new boolean setting, difftool.tabbed, which\nallows a user to view diffs in single window at one go. The `--tabbed`\ncommand line option and the GIT_DIFFTOOL_TABBED environment variable are\nalso available. For now, it works only with vimdiff and related, meld.\n\nWe call to a new invented function, diff_combo_cmd, if it is provided by\nthe diff tool, and pass to it a list of pairs of compared files. The\nlist is available through third file descriptor, filenames are separated\nby a line feed, '\\n', so they can be split by means of unmodified $IFS.\nThe function may close that descriptor after reading the list.\n\nSigned-off-by: Nicholas Guriev <nicholas@guriev.su>\n---\n builtin/difftool.c    |  7 +++++-\n diff.c                | 10 ++++++++-\n git-mergetool--lib.sh | 51 ++++++++++++++++++++++++++++++++++++++-----\n mergetools/meld       |  8 +++++++\n mergetools/vimdiff    | 21 ++++++++++++++++++\n 5 files changed, 90 insertions(+), 7 deletions(-)\n\ndiff --git a/builtin/difftool.c b/builtin/difftool.c\nindex 6e18e623fd..f061d3c029 100644\n--- a/builtin/difftool.c\n+++ b/builtin/difftool.c\n@@ -689,7 +689,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, no_index = 0;\n+\t    tabbed = -1, tool_help = 0, no_index = 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@@ -708,6 +708,8 @@ int cmd_difftool(int argc, const char **argv, const char *prefix)\n \t\tOPT_BOOL(0, \"tool-help\", &tool_help,\n \t\t\t N_(\"print a list of diff tools that may be used with \"\n \t\t\t    \"`--tool`\")),\n+\t\tOPT_BOOL_F(0, \"tabbed\", &tabbed,\n+\t\t\t   N_(\"use tabs in diff tool if supported\"), 0),\n \t\tOPT_BOOL(0, \"trust-exit-code\", &trust_exit_code,\n \t\t\t N_(\"make 'git-difftool' exit when an invoked diff \"\n \t\t\t    \"tool returns a non - zero exit code\")),\n@@ -756,6 +758,9 @@ int cmd_difftool(int argc, const char **argv, const char *prefix)\n \t\t\tdie(_(\"no <cmd> given for --extcmd=<cmd>\"));\n \t}\n \n+\tif (tabbed >= 0)\n+\t\tsetenv(\"GIT_DIFFTOOL_TABBED\", tabbed ? \"true\" : \"false\", 1);\n+\n \tsetenv(\"GIT_DIFFTOOL_TRUST_EXIT_CODE\",\n \t       trust_exit_code ? \"true\" : \"false\", 1);\n \ndiff --git a/diff.c b/diff.c\nindex 2253ec8802..2868848bba 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -1730,6 +1730,14 @@ static struct diff_tempfile *claim_diff_tempfile(void)\n \tBUG(\"diff is failing to clean up its tempfiles\");\n }\n \n+static void forget_tempfile(void)\n+{\n+\tfor (int i = 0; i < ARRAY_SIZE(diff_temp); i++) {\n+\t\tclose_tempfile_gently(diff_temp[i].tempfile);\n+\t\tdiff_temp[i] = (struct diff_tempfile){0};\n+\t}\n+}\n+\n static void remove_tempfile(void)\n {\n \tint i;\n@@ -4275,7 +4283,7 @@ static void run_external_diff(const char *pgm,\n \tif (run_command_v_opt_cd_env(argv.v, RUN_USING_SHELL, NULL, env.v))\n \t\tdie(_(\"external diff died, stopping at %s\"), name);\n \n-\tremove_tempfile();\n+\tforget_tempfile();\n \tstrvec_clear(&argv);\n \tstrvec_clear(&env);\n }\ndiff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh\nindex 78f3647ed9..fef9289618 100644\n--- a/git-mergetool--lib.sh\n+++ b/git-mergetool--lib.sh\n@@ -151,19 +151,25 @@ setup_tool () {\n \n \t# Fallback definitions, to be overridden by tools.\n \tcan_merge () {\n-\t\treturn 0\n+\t\ttrue\n \t}\n \n \tcan_diff () {\n-\t\treturn 0\n+\t\ttrue\n \t}\n \n \tdiff_cmd () {\n-\t\treturn 1\n+\t\tfalse\n+\t}\n+\n+\t# The concrete diff tool may define that it supports combined\n+\t# invocations and the diff_combo_cmd function may be called.\n+\tdiff_combo_supported () {\n+\t\tfalse\n \t}\n \n \tmerge_cmd () {\n-\t\treturn 1\n+\t\tfalse\n \t}\n \n \ttranslate_merge_tool_path () {\n@@ -250,6 +256,28 @@ trust_exit_code () {\n \tfi\n }\n \n+# Check whether tabbed mode is requested and current loaded tool supports it.\n+is_difftool_tabbed () {\n+\t: \"${GIT_DIFFTOOL_TABBED=$(git config --type=bool \\\n+\t\t--default=false difftool.tabbed || echo bad value)}\"\n+\tcase $(printf \"%s\" \"$GIT_DIFFTOOL_TABBED\" | tr '[:upper:]' '[:lower:]') in\n+\tyes|on|true|1)\n+\t\tGIT_DIFFTOOL_TABBED=true\n+\t\t;;\n+\tno|off|false|0|'')\n+\t\tGIT_DIFFTOOL_TABBED=false\n+\t\t;;\n+\t*)\n+\t\techo \"error: bad boolean value of GIT_DIFFTOOL_TABBED\" >&2\n+\t\texit 1\n+\t\t;;\n+\tesac\n+\n+\ttest \"$GIT_DIFFTOOL_TABBED\" = true &&\n+\ttest \"${GIT_DIFF_PATH_TOTAL=0}\" -gt 1 &&\n+\tdiff_combo_supported\n+}\n+\n \n # Entry point for running tools\n run_merge_tool () {\n@@ -274,7 +302,20 @@ run_merge_tool () {\n \n # Run a either a configured or built-in diff tool\n run_diff_cmd () {\n-\tdiff_cmd \"$1\"\n+\tif is_difftool_tabbed\n+\tthen\n+\t\ttemp_file=\"${TMPDIR:-/tmp}/git-${PPID}_tabbed-queue\"\n+\t\ttest \"$GIT_DIFF_PATH_COUNTER\" -eq 1 && >\"$temp_file\"\n+\t\tprintf '%s\\n' \"$LOCAL\" \"$REMOTE\" >>\"$temp_file\"\n+\n+\t\tif test \"$GIT_DIFF_PATH_COUNTER\" -eq \"$GIT_DIFF_PATH_TOTAL\"\n+\t\tthen\n+\t\t\tdiff_combo_cmd 3<\"$temp_file\"\n+\t\t\trm -f -- \"$temp_file\"\n+\t\tfi\n+\telse\n+\t\tdiff_cmd \"$1\"\n+\tfi\n }\n \n # Run a either a configured or built-in merge tool\ndiff --git a/mergetools/meld b/mergetools/meld\nindex aab4ebb935..e2b9d456c1 100644\n--- a/mergetools/meld\n+++ b/mergetools/meld\n@@ -2,6 +2,14 @@ diff_cmd () {\n \t\"$merge_tool_path\" \"$LOCAL\" \"$REMOTE\"\n }\n \n+diff_combo_cmd () {\n+\t\"$merge_tool_path\" $(printf -- '--diff\\n%s\\n%s\\n' `cat <&3`) 3<&-\n+}\n+\n+diff_combo_supported () {\n+\ttrue\n+}\n+\n merge_cmd () {\n \tcheck_meld_for_features\n \ndiff --git a/mergetools/vimdiff b/mergetools/vimdiff\nindex abc8ce4ec4..fe1923076b 100644\n--- a/mergetools/vimdiff\n+++ b/mergetools/vimdiff\n@@ -3,6 +3,27 @@ diff_cmd () {\n \t\t-c 'wincmd l' -c 'cd $GIT_PREFIX' \"$LOCAL\" \"$REMOTE\"\n }\n \n+multitabbed_script='\n+\tlet i = 1\n+\twhile i < argc()\n+\t\texecute \"tabedit\" fnameescape(argv(i - 1))\n+\t\texecute \"diffsplit\" fnameescape(argv(i))\n+\t\twincmd L\n+\t\tlet i = i + 2\n+\tendwhile\n+\ttabfirst\n+\ttabclose\n+\tunlet i\n+\tchdir $GIT_PREFIX\n+'\n+diff_combo_cmd () {\n+\t\"$merge_tool_path\" -R -f -c \"$multitabbed_script\" -- `cat <&3` 3<&-\n+}\n+\n+diff_combo_supported () {\n+\ttrue\n+}\n+\n merge_cmd () {\n \tcase \"$1\" in\n \t*vimdiff)\n-- \n2.27.0\n\n"},{"id":"415240","messageId":"20210125212132.894458-1-nicholas@guriev.su","threadId":"54981","inReplyTo":"20210118210003.3071205-1-nicholas@guriev.su","subject":"[PATCH v3 0/4] difftools in tabbed mode","fromName":"Nicholas Guriev","fromEmail":"nicholas@guriev.su","sentAt":"2021-01-25T21:21:28Z","receivedAt":"2021-01-25T21:25:19Z","isPatch":true,"sender":{"key":"nicholas@guriev.su","avatar":null},"body":"Junio C Hamano, thank you for the comment. I added the diff_combo_supported\nfunction as you suggested. Though it calls to the `true` or the `false` builtins\nas I find them much clearer than the \"return\" command with inverted status. It\nis really confusing that C and most other programming languages adequately\ninterpret non-zero as truth value and zero as falsity value unlike Unix Shell.\nAre such changes acceptable?\n\nAs regards broken tests, it may seem silly but that error was due to a single\nmissing colon. I corrected the mistake and added some automated tests of this\nnew feature.\n\ndiff --git a/git-difftool--helper.sh b/git-difftool--helper.sh\nindex 529d55c96d..85a6dc9c30 100755\n--- a/git-difftool--helper.sh\n+++ b/git-difftool--helper.sh\n@@ -31,7 +31,7 @@ prompt_before_launch () {\n \tdo\n \t\tprintf \"Launch '%s' [Y/n]? \" \"${GIT_DIFFTOOL_EXTCMD:-$merge_tool}\"\n \t\tread ans 2>/dev/null || return 1\n-\t\tcase \"${ans-y}\" in\n+\t\tcase \"${ans:-y}\" in\n \t\t[yY]*) return 0 ;;\n \t\t[nN]*) return 1 ;;\n \t\tesac\n\nChanges compared to v2:\n * Introduced the diff_combo_supported function.\n * Fixed default answer to difftool prompting.\n * More docs about the diff tool mechanism.\n * New auto-tests of the feature.\n\nNicholas Guriev (4):\n  mergetools: support difftool.tabbed setting\n  difftool-helper: conciliate difftool.tabbed and difftool.prompt\n    settings\n  doc: describe new difftool.tabbed feature\n  t7800: new tests of difftool.tabbed feature\n\n Documentation/config/difftool.txt    |   6 +\n Documentation/git-difftool.txt       |  19 ++-\n Documentation/git-mergetool--lib.txt |  62 ++++++++++\n Documentation/git.txt                |   8 ++\n builtin/difftool.c                   |   7 +-\n diff.c                               |  10 +-\n git-difftool--helper.sh              |  39 ++++--\n git-mergetool--lib.sh                |  71 ++++++++++-\n mergetools/meld                      |   8 ++\n mergetools/vimdiff                   |  21 ++++\n t/t7800-difftool.sh                  | 175 ++++++++++++++++++++++++++-\n 11 files changed, 401 insertions(+), 25 deletions(-)\n\n-- \n2.27.0\n\n"},{"id":"415241","messageId":"20210125212132.894458-3-nicholas@guriev.su","threadId":"54981","inReplyTo":"20210125212132.894458-1-nicholas@guriev.su","subject":"[PATCH v3 2/4] difftool-helper: conciliate difftool.tabbed and difftool.prompt settings","fromName":"Nicholas Guriev","fromEmail":"nicholas@guriev.su","sentAt":"2021-01-25T21:21:30Z","receivedAt":"2021-01-25T21:25:19Z","isPatch":true,"sender":{"key":"nicholas@guriev.su","avatar":null},"body":"The commit combines paths of compared files into a separate temporary\nfile and prints them before launch the tool on last invocation of the\nhelper. All temporary files will be removed before exiting after that.\nAt least, we try. But it may happen the files remain in case of a bug\nor a random SIGKILL.\n\nSigned-off-by: Nicholas Guriev <nicholas@guriev.su>\n---\n git-difftool--helper.sh | 39 ++++++++++++++++++++++++++-------------\n git-mergetool--lib.sh   | 34 +++++++++++++++++++++++++++-------\n 2 files changed, 53 insertions(+), 20 deletions(-)\n\ndiff --git a/git-difftool--helper.sh b/git-difftool--helper.sh\nindex 46af3e60b7..85a6dc9c30 100755\n--- a/git-difftool--helper.sh\n+++ b/git-difftool--helper.sh\n@@ -11,8 +11,8 @@ TOOL_MODE=diff\n # difftool.prompt controls the default prompt/no-prompt behavior\n # and is overridden with $GIT_DIFFTOOL*_PROMPT.\n should_prompt () {\n-\tprompt_merge=$(git config --bool mergetool.prompt || echo true)\n-\tprompt=$(git config --bool difftool.prompt || echo $prompt_merge)\n+\tprompt=$(git config --bool mergetool.prompt || echo true)\n+\tprompt=$(git config --bool difftool.prompt || echo $prompt)\n \tif test \"$prompt\" = true\n \tthen\n \t\ttest -z \"$GIT_DIFFTOOL_NO_PROMPT\"\n@@ -26,6 +26,18 @@ use_ext_cmd () {\n \ttest -n \"$GIT_DIFFTOOL_EXTCMD\"\n }\n \n+prompt_before_launch () {\n+\twhile true\n+\tdo\n+\t\tprintf \"Launch '%s' [Y/n]? \" \"${GIT_DIFFTOOL_EXTCMD:-$merge_tool}\"\n+\t\tread ans 2>/dev/null || return 1\n+\t\tcase \"${ans:-y}\" in\n+\t\t[yY]*) return 0 ;;\n+\t\t[nN]*) return 1 ;;\n+\t\tesac\n+\tdone\n+}\n+\n launch_merge_tool () {\n \t# Merged is the filename as it appears in the work tree\n \t# Local is the contents of a/filename\n@@ -40,19 +52,20 @@ launch_merge_tool () {\n \t# the user with the real $MERGED name before launching $merge_tool.\n \tif should_prompt\n \tthen\n-\t\tprintf \"\\nViewing (%s/%s): '%s'\\n\" \"$GIT_DIFF_PATH_COUNTER\" \\\n-\t\t\t\"$GIT_DIFF_PATH_TOTAL\" \"$MERGED\"\n-\t\tif use_ext_cmd\n+\t\tappend_templist merged-paths \"$MERGED\"\n+\t\t# Do preinit before test whether the tool supports tabbed run.\n+\t\tuse_ext_cmd || setup_tool \"$merge_tool\"\n+\n+\t\tif ! is_difftool_tabbed\n \t\tthen\n-\t\t\tprintf \"Launch '%s' [Y/n]? \" \\\n-\t\t\t\t\"$GIT_DIFFTOOL_EXTCMD\"\n-\t\telse\n-\t\t\tprintf \"Launch '%s' [Y/n]? \" \"$merge_tool\"\n-\t\tfi\n-\t\tread ans || return\n-\t\tif test \"$ans\" = n\n+\t\t\tprintf \"\\nViewing (%d/%d): '%s'\\n\" \"$GIT_DIFF_PATH_COUNTER\" \\\n+\t\t\t\t\"$GIT_DIFF_PATH_TOTAL\" \"$MERGED\"\n+\t\t\tprompt_before_launch || return\n+\t\telif on_last_file\n \t\tthen\n-\t\t\treturn\n+\t\t\tprintf \"Viewing %d files:\\n\" \"$GIT_DIFF_PATH_TOTAL\"\n+\t\t\tprintf \"  '%s'\\n\" `cat \"$LAST_TEMPFILE\"`\n+\t\t\tprompt_before_launch || return\n \t\tfi\n \tfi\n \ndiff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh\nindex fef9289618..c29abfa4fb 100644\n--- a/git-mergetool--lib.sh\n+++ b/git-mergetool--lib.sh\n@@ -22,6 +22,10 @@ is_available () {\n \ttype \"$merge_tool_path\" >/dev/null 2>&1\n }\n \n+on_last_file () {\n+\ttest \"$GIT_DIFF_PATH_COUNTER\" -eq \"$GIT_DIFF_PATH_TOTAL\" 2>/dev/null\n+}\n+\n list_config_tools () {\n \tsection=$1\n \tline_prefix=${2:-}\n@@ -278,6 +282,26 @@ is_difftool_tabbed () {\n \tdiff_combo_supported\n }\n \n+# Save lines to the chosen temporary file for usage from subsequent invocations.\n+# Path to the file will be placed to the LAST_TEMPFILE variable for later links.\n+append_templist () {\n+\tLAST_TEMPFILE=\"${TMPDIR:-/tmp}/git-${PPID}_$1\"\n+\tshift\n+\n+\tif test \"$GIT_DIFF_PATH_COUNTER\" -eq 1\n+\tthen\n+\t\tprintf '%s\\n' \"$@\" >\"$LAST_TEMPFILE\"\n+\telse\n+\t\tprintf '%s\\n' \"$@\" >>\"$LAST_TEMPFILE\"\n+\tfi\n+}\n+\n+# Clean any temporary files that may create this script.\n+clean_templists () {\n+\trm -f -- \"${TMPDIR:-/tmp}/git-${PPID}\"_*\n+}\n+trap 'on_last_file && clean_templists' EXIT INT TERM\n+\n \n # Entry point for running tools\n run_merge_tool () {\n@@ -304,14 +328,10 @@ run_merge_tool () {\n run_diff_cmd () {\n \tif is_difftool_tabbed\n \tthen\n-\t\ttemp_file=\"${TMPDIR:-/tmp}/git-${PPID}_tabbed-queue\"\n-\t\ttest \"$GIT_DIFF_PATH_COUNTER\" -eq 1 && >\"$temp_file\"\n-\t\tprintf '%s\\n' \"$LOCAL\" \"$REMOTE\" >>\"$temp_file\"\n-\n-\t\tif test \"$GIT_DIFF_PATH_COUNTER\" -eq \"$GIT_DIFF_PATH_TOTAL\"\n+\t\tappend_templist tabbed-queue \"$LOCAL\" \"$REMOTE\"\n+\t\tif on_last_file\n \t\tthen\n-\t\t\tdiff_combo_cmd 3<\"$temp_file\"\n-\t\t\trm -f -- \"$temp_file\"\n+\t\t\tdiff_combo_cmd 3<\"$LAST_TEMPFILE\"\n \t\tfi\n \telse\n \t\tdiff_cmd \"$1\"\n-- \n2.27.0\n\n"},{"id":"415244","messageId":"xmqqa6swy5y6.fsf@gitster.c.googlers.com","threadId":"54981","inReplyTo":"20210125212132.894458-3-nicholas@guriev.su","subject":"Re: [PATCH v3 2/4] difftool-helper: conciliate difftool.tabbed and difftool.prompt settings","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-01-25T23:04:01Z","receivedAt":"2021-01-25T23:06:23Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nicholas Guriev <nicholas@guriev.su> writes:\n\n> The commit combines paths of compared files into a separate temporary\n> file and prints them before launch the tool on last invocation of the\n> helper. All temporary files will be removed before exiting after that.\n> At least, we try. But it may happen the files remain in case of a bug\n> or a random SIGKILL.\n>\n> Signed-off-by: Nicholas Guriev <nicholas@guriev.su>\n> ---\n\nIsn't the introduction of prompt_before_launch etc. would be a\nwelcome change before step [v3 1/4]?  If it makes a pure improvement\nof the existing code without adding any new/additional feature like\nthe \"tabbed\" stuff, it would be better be discussed before anything\nelse in the series.\n\nThen the new \"tabbed\" stuff can take advantage of the cleaned up\ncode with the new helpers---the patch that introduces the feature\nwill become more concise and easier to follow, I would think.\n\nIf I am reading the patches right, [v3 1&2/4] goes the other way\naround: \"let's add a new feature first in an ad-hoc way.  ah, it\nseems that the code added for the new feature can be simplified if\nwe tweak a few things, and while we are doing so, we can adjust old\nfeature to use the same simplification again\".\n\nRegarding what [v3 1/4] does (which if we follow thru the above\nsuggestion would become [v4 2/4]), I am not sure that we want to add\n'if tabbed mode, do this, otherwise do that' to run_diff_cmd.  It\nwould become awkward to keep piling such a change on top in the\nfunction in the longer run.  I am wondering if each mergetool can\noverride run_diff_cmd as a whole, when it is dot-sourced to set up\nitself.  The logic you added inside is_difftool_tabbed in [v3 1/4]\nwould probably need to be made available to the dot-sourced backends\nso that they can just call something without reimplementing the same\nlogic, but by doing so, diff_combo_supported would become unneeded.\nTools that do not support the \"combo\" mode just do not have to do\nanything.\n\nBut such a change would become easier to review if we do restructure\nlike this [v3 2/4] does earlier in the series _before_ a new feature\ngets introduced.\n\nThanks.\n"},{"id":"416782","messageId":"CAJDDKr4zM1ZuNc+JpQnAtqwa5Ljv7_5bL3X-cC3e5Xg3z2Cbcw@mail.gmail.com","threadId":"54981","inReplyTo":"2fb58fd30ae730ccd3e88ec51b5fe6d80ab7a8c7.camel@guriev.su","subject":"Re: [RFC PATCH] mergetools: support difftool.tabbed setting","fromName":"David Aguilar","fromEmail":"davvid@gmail.com","sentAt":"2021-02-12T05:51:24Z","receivedAt":"2021-02-12T05:53:07Z","isPatch":true,"sender":{"key":"davvid@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13196?v=4"},"body":"On Tue, Jan 12, 2021 at 10:07 PM Nicholas Guriev <nicholas@guriev.su> wrote:\n>\n> I was asked how to configure \"git difftool\" to open files using several\n> tabs and stop spawning diff application on every modified file. I looked\n> into Git source and found no possibility to run diff tool at one step.\n>\n> The patch allows a user to view diffs in single window at one go. The\n> current implementation is still poor and it can be used solely for\n> demonstration purposes. To see it in action, tweak the local gitconfig:\n>\n>     git config difftool.prompt false\n>     git config difftool.tabbed true\n>\n> Then run:\n>\n>     git difftool -t vimdiff\n>\n> Or:\n>\n>     git difftool -t meld\n>\n> The solution has some restrictions, diffing up to ten files works now (I\n> did not bother with dynamic memory allocation), and it does not handle spaces\n> in file names (I do not know how to pass them correctly to underlying tools\n> without \"xargs -0\").\n>\n> I think the git-difftool--helper should be changed so that it could\n> process many files in single invocation and it would not use a temporary\n> file by itself. A similar behaviour can be done in git-mergetool, too.\n>\n> Do you have ideas how to better implement such a feature? Any comments\n> are welcome.\n>\n> P.S.: I'm attaching screenshots for a clear demo what I mean.\n> ---\n>  diff.c                |  4 ++--\n>  git-mergetool--lib.sh | 36 +++++++++++++++++++++++++++++++++++-\n>  mergetools/meld       |  4 ++++\n>  mergetools/vimdiff    | 17 +++++++++++++++++\n>  4 files changed, 58 insertions(+), 3 deletions(-)\n>\n>\n>\n\nI'm not really sure if \"tabbed\" is the best name for what's going on,\nthough.  It's really more of a \"diff everything in one shot\" mode, and\nit just so happens that the tools in question use tabs.\n\nGeneral note -- similar to the convention followed by\nmergetool.hideResolved and other difftool things I think it would make\nsense for tools to be able to override this on a per-tool basis.\n\nThat said, I wonder whether we need this new feature, or whether we\nshould instead improve an existing one.  I'm leaning towards improving\nthe existing dir-diff feature as a better alternative.\n\nIt's unfortunate that the \"git difftool --dir-diff\" feature doesn't\nseem to mesh well with vimdiff.  It does work well with other tools\nthat support diffing arbitrary directories, notably meld, xxdiff, etc.\n\nRegarding vimdiff + git difftool -d, there is this advice:\nhttps://stackoverflow.com/questions/8156493/git-vimdiff-and-dirdiff\n\nmeld works just fine with \"git difftool -d\" (arguably nicer, since it\ngives you a directory view and a diff view in separate tabs), so if\nthe only improvement is for vimdiff, then maybe the advice with the\nDirDiff plugin might be a better way to go.\n\nHaving something like a \"difftool.vimdiff.useDirDiff\" configuration\nvariable could be a way for us to adopt the advice that they offer\nthere.  We could have the dir-diff difftool mode set a variable that\nthe vimdiff scriptlet could use to detect that we're in dir-diff mode.\nThen, when that variable is set, vimdiff could use,\n\n    vim -f '+next' '+execute \\\"DirDiff\\\" argv(0) argv(1)' $LOCAL $REMOTE\n\n(as mentioned in the SO page) to invoke dir-diff mode in vim.\n\nThat way the user only needs to set:\n\ngit config difftool.vimdiff.useDirDiff\n\n... and then \"git diffftool -d\" will integrate with the DirDiff plugin.\n\nWhat do you think about improving the vimdiff scriptlet to better\nintegrate with \"git difftool -d\" instead?\n-- \nDavid\n"},{"id":"416805","messageId":"xmqqmtw9udue.fsf@gitster.c.googlers.com","threadId":"54981","inReplyTo":"CAJDDKr4zM1ZuNc+JpQnAtqwa5Ljv7_5bL3X-cC3e5Xg3z2Cbcw@mail.gmail.com","subject":"Re: [RFC PATCH] mergetools: support difftool.tabbed setting","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-02-12T22:21:29Z","receivedAt":"2021-02-12T22:22:30Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"David Aguilar <davvid@gmail.com> writes:\n\n> I'm not really sure if \"tabbed\" is the best name for what's going on,\n> though.  It's really more of a \"diff everything in one shot\" mode, and\n> it just so happens that the tools in question use tabs.\n\nThis statement matches my reaction to this new feature exactly.\n\nThe way the external commands are triggered via GIT_EXTERNAL_DIFF\nmechanism makes it \"easy\" to show changes for one path at a time and\n\"hard\" to do so for all paths at once, but the resulting end-user\nexperience that is forced to view one path at a time may be awkward.\n\n> That said, I wonder whether we need this new feature, or whether we\n> should instead improve an existing one.  I'm leaning towards improving\n> the existing dir-diff feature as a better alternative. ...\n\nAs a non-user, I have no strong opinion on the \"new feature\"; other\nthan that I trust your judgement on the \"difftool\" design issues,\nthat is.\n\nThanks.\n"}]}