[RFC PATCH] mergetools: support difftool.tabbed setting

[RFC PATCH] mergetools: support difftool.tabbed setting

[Nicholas Guriev]

[-- Attachment #1.1: Type: text/plain, Size: 1457 bytes --]

I was asked how to configure "git difftool" to open files using several
tabs and stop spawning diff application on every modified file. I looked
into Git source and found no possibility to run diff tool at one step.

The patch allows a user to view diffs in single window at one go. The
current implementation is still poor and it can be used solely for
demonstration purposes. To see it in action, tweak the local gitconfig:

    git config difftool.prompt false
    git config difftool.tabbed true

Then run:

    git difftool -t vimdiff

Or:

    git difftool -t meld

The solution has some restrictions, diffing up to ten files works now (I
did not bother with dynamic memory allocation), and it does not handle spaces
in file names (I do not know how to pass them correctly to underlying tools
without "xargs -0").

I think the git-difftool--helper should be changed so that it could
process many files in single invocation and it would not use a temporary
file by itself. A similar behaviour can be done in git-mergetool, too.

Do you have ideas how to better implement such a feature? Any comments
are welcome.

P.S.: I'm attaching screenshots for a clear demo what I mean.
---
 diff.c                |  4 ++--
 git-mergetool--lib.sh | 36 +++++++++++++++++++++++++++++++++++-
 mergetools/meld       |  4 ++++
 mergetools/vimdiff    | 17 +++++++++++++++++
 4 files changed, 58 insertions(+), 3 deletions(-)




[-- Attachment #1.2: 0001-mergetools-support-difftool.tabbed-setting.patch --]
[-- Type: text/x-patch, Size: 3035 bytes --]

diff --git a/diff.c b/diff.c
index 2253ec880..8a265e0b0 100644
--- a/diff.c
+++ b/diff.c
@@ -532,7 +532,7 @@ static struct diff_tempfile {
 	 * this tempfile object is used to manage its lifetime.
 	 */
 	struct tempfile *tempfile;
-} diff_temp[2];
+} diff_temp[20];
 
 struct emit_callback {
 	int color_diff;
@@ -4275,7 +4275,7 @@ static void run_external_diff(const char *pgm,
 	if (run_command_v_opt_cd_env(argv.v, RUN_USING_SHELL, NULL, env.v))
 		die(_("external diff died, stopping at %s"), name);
 
-	remove_tempfile();
+	//remove_tempfile();
 	strvec_clear(&argv);
 	strvec_clear(&env);
 }
diff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh
index 7225abd81..e599e4243 100644
--- a/git-mergetool--lib.sh
+++ b/git-mergetool--lib.sh
@@ -193,6 +193,8 @@ setup_tool () {
 		false
 	}
 
+	unset -f diff_combo_cmd
+
 	if test -f "$MERGE_TOOLS_DIR/$tool"
 	then
 		. "$MERGE_TOOLS_DIR/$tool"
@@ -248,6 +250,25 @@ trust_exit_code () {
 	fi
 }
 
+is_difftool_tabbed () {
+	: "${GIT_DIFFTOOL_TABBED:=$(git config --type=bool --default=false difftool.tabbed)}"
+	case $(printf "%s" "$GIT_DIFFTOOL_TABBED" | tr '[:upper:]' '[:lower:]') in
+	yes|on|true|1)
+		GIT_DIFFTOOL_TABBED=true
+		;;
+	no|off|false|0)
+		GIT_DIFFTOOL_TABBED=false
+		;;
+	*)
+		echo "error: bad boolean value of GIT_DIFFTOOL_TABBED" >&2
+		exit 1
+		;;
+	esac
+
+	test "$GIT_DIFFTOOL_TABBED" = true && test "$GIT_DIFF_PATH_TOTAL" -gt 1 \
+		&& type diff_combo_cmd >/dev/null 2>&1
+}
+
 
 # Entry point for running tools
 run_merge_tool () {
@@ -272,7 +293,20 @@ run_merge_tool () {
 
 # Run a either a configured or built-in diff tool
 run_diff_cmd () {
-	diff_cmd "$1"
+	if is_difftool_tabbed
+	then
+		temp_file="${TMPDIR:-/tmp}/git-${PPID}_tabbed-queue"
+		test "$GIT_DIFF_PATH_COUNTER" -eq 1 && > "$temp_file"
+		printf "%s " "$LOCAL" "$REMOTE" >> "$temp_file"
+
+		if [ "$GIT_DIFF_PATH_COUNTER" -eq "$GIT_DIFF_PATH_TOTAL" ]
+		then
+			diff_combo_cmd 3< "$temp_file"
+			rm -f -- "$temp_file"
+		fi
+	else
+		diff_cmd "$1"
+	fi
 }
 
 # Run a either a configured or built-in merge tool
diff --git a/mergetools/meld b/mergetools/meld
index aab4ebb93..6570bf0f8 100644
--- a/mergetools/meld
+++ b/mergetools/meld
@@ -2,6 +2,10 @@ diff_cmd () {
 	"$merge_tool_path" "$LOCAL" "$REMOTE"
 }
 
+diff_combo_cmd () {
+	( IFS=' '; "$merge_tool_path" $(printf ' --diff %s %s' `cat <&3`) 3<&- )
+}
+
 merge_cmd () {
 	check_meld_for_features
 
diff --git a/mergetools/vimdiff b/mergetools/vimdiff
index abc8ce4ec..31d6e1eaa 100644
--- a/mergetools/vimdiff
+++ b/mergetools/vimdiff
@@ -3,6 +3,23 @@ diff_cmd () {
 		-c 'wincmd l' -c 'cd $GIT_PREFIX' "$LOCAL" "$REMOTE"
 }
 
+multitabbed_script='
+	let i = 1
+	while i < argc()
+		execute "tabedit" fnameescape(argv(i - 1))
+		execute "diffsplit" fnameescape(argv(i))
+		wincmd L
+		let i = i + 2
+	endwhile
+	tabfirst
+	tabclose
+	unlet i
+'
+diff_combo_cmd () {
+	( IFS=' '; cd "$GIT_PREFIX" && "$merge_tool_path" -R -f \
+		-c "$multitabbed_script" `cat <&3` 3<&- )
+}
+
 merge_cmd () {
 	case "$1" in
 	*vimdiff)

[-- Attachment #1.3: meld_screenshot-1.png --]
[-- Type: image/png, Size: 58533 bytes --]

[-- Attachment #1.4: vimdiff_screenshot-2.png --]
[-- Type: image/png, Size: 42323 bytes --]

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[RFC PATCH v2 0/3] implement tabbed mode in difftools

[Nicholas Guriev]

In this mode the "git difftool" command opens all compared files via single
invocation of an editor passing to it entire list of the changed files. I find
it useful, and it allows a user to switch easily forward and back between the
files. You will find some screenshots demonstrating the feature in my previous
message.

The patch series looks better now, and the two problems mentioned earlier have
been solved. I added a new static function, forget_tempfile, into the diff.c
file. It cleans diff_temp structures and do not remove the temporary files.
They anyway would be deleted in an atexit handler. In this way, I can collect
content of all changed files before running the editor.

For solving the second problem with spaces, I am separating file names with
line-feeds, '\n', that are less common. This restriction is the same with the
"git mergetool" command. I think it is acceptable at the present stage.

I have also repaired prompting in the tabbed mode. In such case, the command
asks a user only once right before starting the editor. And I have described
briefly the new changes to the best of my ability.

Alas, some other problems still remain. First, some automated tests failed that
related to the difftool.prompt setting. I have modified its behavior a bit and
it now contains a loop. Second, there are no new tests, yet I am going to add
them later. And third, the main remaining problem is the method of calling of
the difftool helper. I do not like that I have to write temporary files in the
helper code to store its state between invocations. But maybe, someone offer a
better solution that is easy to incorporate into the current architecture.

In conclusion, the patches are still not ready for merging, and any comments or
testing are welcome.


Nicholas Guriev (3):
  mergetools: support difftool.tabbed setting
  difftool-helper: conciliate difftool.tabbed and difftool.prompt
    settings
  doc: describe new difftool.tabbed feature

 Documentation/config/difftool.txt |  6 +++
 Documentation/git-difftool.txt    | 19 ++++++++--
 Documentation/git.txt             |  4 ++
 builtin/difftool.c                |  7 +++-
 diff.c                            | 10 ++++-
 git-difftool--helper.sh           | 39 ++++++++++++-------
 git-mergetool--lib.sh             | 62 ++++++++++++++++++++++++++++++-
 mergetools/meld                   |  4 ++
 mergetools/vimdiff                | 17 +++++++++
 9 files changed, 148 insertions(+), 20 deletions(-)

-- 
2.27.0

[RFC PATCH v2 1/3] mergetools: support difftool.tabbed setting

[Nicholas Guriev]

The patch introduces a new boolean setting, difftool.tabbed, which
allows a user to view diffs in single window at one go. The `--tabbed`
command line option and the GIT_DIFFTOOL_TABBED environment variable are
also available. For now, it works only with vimdiff and related, meld.

We call to a new invented function, diff_combo_cmd, if it is provided by
the diff tool, and pass to it a list of pairs of compared files. The
list is available through third file descriptor, filenames are separated
by a line feed, '\n', so they can be split by means of unmodified $IFS.
The function may close that descriptor after reading the list.
---
 builtin/difftool.c    |  7 ++++++-
 diff.c                | 10 +++++++++-
 git-mergetool--lib.sh | 42 +++++++++++++++++++++++++++++++++++++++++-
 mergetools/meld       |  4 ++++
 mergetools/vimdiff    | 17 +++++++++++++++++
 5 files changed, 77 insertions(+), 3 deletions(-)

diff --git a/builtin/difftool.c b/builtin/difftool.c
index 6e18e623fd..f061d3c029 100644
--- a/builtin/difftool.c
+++ b/builtin/difftool.c
@@ -689,7 +689,7 @@ static int run_file_diff(int prompt, const char *prefix,
 int cmd_difftool(int argc, const char **argv, const char *prefix)
 {
 	int use_gui_tool = 0, dir_diff = 0, prompt = -1, symlinks = 0,
-	    tool_help = 0, no_index = 0;
+	    tabbed = -1, tool_help = 0, no_index = 0;
 	static char *difftool_cmd = NULL, *extcmd = NULL;
 	struct option builtin_difftool_options[] = {
 		OPT_BOOL('g', "gui", &use_gui_tool,
@@ -708,6 +708,8 @@ int cmd_difftool(int argc, const char **argv, const char *prefix)
 		OPT_BOOL(0, "tool-help", &tool_help,
 			 N_("print a list of diff tools that may be used with "
 			    "`--tool`")),
+		OPT_BOOL_F(0, "tabbed", &tabbed,
+			   N_("use tabs in diff tool if supported"), 0),
 		OPT_BOOL(0, "trust-exit-code", &trust_exit_code,
 			 N_("make 'git-difftool' exit when an invoked diff "
 			    "tool returns a non - zero exit code")),
@@ -756,6 +758,9 @@ int cmd_difftool(int argc, const char **argv, const char *prefix)
 			die(_("no <cmd> given for --extcmd=<cmd>"));
 	}
 
+	if (tabbed >= 0)
+		setenv("GIT_DIFFTOOL_TABBED", tabbed ? "true" : "false", 1);
+
 	setenv("GIT_DIFFTOOL_TRUST_EXIT_CODE",
 	       trust_exit_code ? "true" : "false", 1);
 
diff --git a/diff.c b/diff.c
index 2253ec8802..2868848bba 100644
--- a/diff.c
+++ b/diff.c
@@ -1730,6 +1730,14 @@ static struct diff_tempfile *claim_diff_tempfile(void)
 	BUG("diff is failing to clean up its tempfiles");
 }
 
+static void forget_tempfile(void)
+{
+	for (int i = 0; i < ARRAY_SIZE(diff_temp); i++) {
+		close_tempfile_gently(diff_temp[i].tempfile);
+		diff_temp[i] = (struct diff_tempfile){0};
+	}
+}
+
 static void remove_tempfile(void)
 {
 	int i;
@@ -4275,7 +4283,7 @@ static void run_external_diff(const char *pgm,
 	if (run_command_v_opt_cd_env(argv.v, RUN_USING_SHELL, NULL, env.v))
 		die(_("external diff died, stopping at %s"), name);
 
-	remove_tempfile();
+	forget_tempfile();
 	strvec_clear(&argv);
 	strvec_clear(&env);
 }
diff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh
index 78f3647ed9..2a1edbb9b9 100644
--- a/git-mergetool--lib.sh
+++ b/git-mergetool--lib.sh
@@ -195,6 +195,11 @@ setup_tool () {
 		false
 	}
 
+	# Clear combo function declared by a previous tool (if any) to
+	# unambiguously indicate that the current one supports the feature or
+	# not.
+	unset -f diff_combo_cmd
+
 	if test -f "$MERGE_TOOLS_DIR/$tool"
 	then
 		. "$MERGE_TOOLS_DIR/$tool"
@@ -250,6 +255,28 @@ trust_exit_code () {
 	fi
 }
 
+# Check whether tabbed mode is requested and current loaded tool supports it.
+is_difftool_tabbed () {
+	: "${GIT_DIFFTOOL_TABBED=$(git config --type=bool \
+		--default=false difftool.tabbed || echo bad value)}"
+	case $(printf "%s" "$GIT_DIFFTOOL_TABBED" | tr '[:upper:]' '[:lower:]') in
+	yes|on|true|1)
+		GIT_DIFFTOOL_TABBED=true
+		;;
+	no|off|false|0|'')
+		GIT_DIFFTOOL_TABBED=false
+		;;
+	*)
+		echo "error: bad boolean value of GIT_DIFFTOOL_TABBED" >&2
+		exit 1
+		;;
+	esac
+
+	test "$GIT_DIFFTOOL_TABBED" = true &&
+	test "${GIT_DIFF_PATH_TOTAL=0}" -gt 1 &&
+	type diff_combo_cmd >/dev/null 2>&1
+}
+
 
 # Entry point for running tools
 run_merge_tool () {
@@ -274,7 +301,20 @@ run_merge_tool () {
 
 # Run a either a configured or built-in diff tool
 run_diff_cmd () {
-	diff_cmd "$1"
+	if is_difftool_tabbed
+	then
+		temp_file="${TMPDIR:-/tmp}/git-${PPID}_tabbed-queue"
+		test "$GIT_DIFF_PATH_COUNTER" -eq 1 && >"$temp_file"
+		printf '%s\n' "$LOCAL" "$REMOTE" >>"$temp_file"
+
+		if test "$GIT_DIFF_PATH_COUNTER" -eq "$GIT_DIFF_PATH_TOTAL"
+		then
+			diff_combo_cmd 3<"$temp_file"
+			rm -f -- "$temp_file"
+		fi
+	else
+		diff_cmd "$1"
+	fi
 }
 
 # Run a either a configured or built-in merge tool
diff --git a/mergetools/meld b/mergetools/meld
index aab4ebb935..2f40262a70 100644
--- a/mergetools/meld
+++ b/mergetools/meld
@@ -2,6 +2,10 @@ diff_cmd () {
 	"$merge_tool_path" "$LOCAL" "$REMOTE"
 }
 
+diff_combo_cmd () {
+	"$merge_tool_path" $(printf -- '--diff\n%s\n%s\n' `cat <&3`) 3<&-
+}
+
 merge_cmd () {
 	check_meld_for_features
 
diff --git a/mergetools/vimdiff b/mergetools/vimdiff
index abc8ce4ec4..006e7b825d 100644
--- a/mergetools/vimdiff
+++ b/mergetools/vimdiff
@@ -3,6 +3,23 @@ diff_cmd () {
 		-c 'wincmd l' -c 'cd $GIT_PREFIX' "$LOCAL" "$REMOTE"
 }
 
+multitabbed_script='
+	let i = 1
+	while i < argc()
+		execute "tabedit" fnameescape(argv(i - 1))
+		execute "diffsplit" fnameescape(argv(i))
+		wincmd L
+		let i = i + 2
+	endwhile
+	tabfirst
+	tabclose
+	unlet i
+	chdir $GIT_PREFIX
+'
+diff_combo_cmd () {
+	"$merge_tool_path" -R -f -c "$multitabbed_script" -- `cat <&3` 3<&-
+}
+
 merge_cmd () {
 	case "$1" in
 	*vimdiff)
-- 
2.27.0

[RFC PATCH v2 2/3] difftool-helper: conciliate difftool.tabbed and difftool.prompt settings

[Nicholas Guriev]

The commit combines paths of compared files into a separate temporary
file and prints them before launch the tool on last invocation of the
helper. All temporary files will be removed before exiting after that.
At least, we try. But it may happen the files remain in case of a bug
or a random SIGKILL.
---
 git-difftool--helper.sh | 39 ++++++++++++++++++++++++++-------------
 git-mergetool--lib.sh   | 34 +++++++++++++++++++++++++++-------
 2 files changed, 53 insertions(+), 20 deletions(-)

diff --git a/git-difftool--helper.sh b/git-difftool--helper.sh
index 46af3e60b7..529d55c96d 100755
--- a/git-difftool--helper.sh
+++ b/git-difftool--helper.sh
@@ -11,8 +11,8 @@ TOOL_MODE=diff
 # difftool.prompt controls the default prompt/no-prompt behavior
 # and is overridden with $GIT_DIFFTOOL*_PROMPT.
 should_prompt () {
-	prompt_merge=$(git config --bool mergetool.prompt || echo true)
-	prompt=$(git config --bool difftool.prompt || echo $prompt_merge)
+	prompt=$(git config --bool mergetool.prompt || echo true)
+	prompt=$(git config --bool difftool.prompt || echo $prompt)
 	if test "$prompt" = true
 	then
 		test -z "$GIT_DIFFTOOL_NO_PROMPT"
@@ -26,6 +26,18 @@ use_ext_cmd () {
 	test -n "$GIT_DIFFTOOL_EXTCMD"
 }
 
+prompt_before_launch () {
+	while true
+	do
+		printf "Launch '%s' [Y/n]? " "${GIT_DIFFTOOL_EXTCMD:-$merge_tool}"
+		read ans 2>/dev/null || return 1
+		case "${ans-y}" in
+		[yY]*) return 0 ;;
+		[nN]*) return 1 ;;
+		esac
+	done
+}
+
 launch_merge_tool () {
 	# Merged is the filename as it appears in the work tree
 	# Local is the contents of a/filename
@@ -40,19 +52,20 @@ launch_merge_tool () {
 	# the user with the real $MERGED name before launching $merge_tool.
 	if should_prompt
 	then
-		printf "\nViewing (%s/%s): '%s'\n" "$GIT_DIFF_PATH_COUNTER" \
-			"$GIT_DIFF_PATH_TOTAL" "$MERGED"
-		if use_ext_cmd
+		append_templist merged-paths "$MERGED"
+		# Do preinit before test whether the tool supports tabbed run.
+		use_ext_cmd || setup_tool "$merge_tool"
+
+		if ! is_difftool_tabbed
 		then
-			printf "Launch '%s' [Y/n]? " \
-				"$GIT_DIFFTOOL_EXTCMD"
-		else
-			printf "Launch '%s' [Y/n]? " "$merge_tool"
-		fi
-		read ans || return
-		if test "$ans" = n
+			printf "\nViewing (%d/%d): '%s'\n" "$GIT_DIFF_PATH_COUNTER" \
+				"$GIT_DIFF_PATH_TOTAL" "$MERGED"
+			prompt_before_launch || return
+		elif on_last_file
 		then
-			return
+			printf "Viewing %d files:\n" "$GIT_DIFF_PATH_TOTAL"
+			printf "  '%s'\n" `cat "$LAST_TEMPFILE"`
+			prompt_before_launch || return
 		fi
 	fi
 
diff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh
index 2a1edbb9b9..4bb0e31dac 100644
--- a/git-mergetool--lib.sh
+++ b/git-mergetool--lib.sh
@@ -22,6 +22,10 @@ is_available () {
 	type "$merge_tool_path" >/dev/null 2>&1
 }
 
+on_last_file () {
+	test "$GIT_DIFF_PATH_COUNTER" -eq "$GIT_DIFF_PATH_TOTAL" 2>/dev/null
+}
+
 list_config_tools () {
 	section=$1
 	line_prefix=${2:-}
@@ -277,6 +281,26 @@ is_difftool_tabbed () {
 	type diff_combo_cmd >/dev/null 2>&1
 }
 
+# Save lines to the chosen temporary file for usage from subsequent invocations.
+# Path to the file will be placed to the LAST_TEMPFILE variable for later links.
+append_templist () {
+	LAST_TEMPFILE="${TMPDIR:-/tmp}/git-${PPID}_$1"
+	shift
+
+	if test "$GIT_DIFF_PATH_COUNTER" -eq 1
+	then
+		printf '%s\n' "$@" >"$LAST_TEMPFILE"
+	else
+		printf '%s\n' "$@" >>"$LAST_TEMPFILE"
+	fi
+}
+
+# Clean any temporary files that may create this script.
+clean_templists () {
+	rm -f -- "${TMPDIR:-/tmp}/git-${PPID}"_*
+}
+trap 'on_last_file && clean_templists' EXIT INT TERM
+
 
 # Entry point for running tools
 run_merge_tool () {
@@ -303,14 +327,10 @@ run_merge_tool () {
 run_diff_cmd () {
 	if is_difftool_tabbed
 	then
-		temp_file="${TMPDIR:-/tmp}/git-${PPID}_tabbed-queue"
-		test "$GIT_DIFF_PATH_COUNTER" -eq 1 && >"$temp_file"
-		printf '%s\n' "$LOCAL" "$REMOTE" >>"$temp_file"
-
-		if test "$GIT_DIFF_PATH_COUNTER" -eq "$GIT_DIFF_PATH_TOTAL"
+		append_templist tabbed-queue "$LOCAL" "$REMOTE"
+		if on_last_file
 		then
-			diff_combo_cmd 3<"$temp_file"
-			rm -f -- "$temp_file"
+			diff_combo_cmd 3<"$LAST_TEMPFILE"
 		fi
 	else
 		diff_cmd "$1"
-- 
2.27.0

[RFC PATCH v2 3/3] doc: describe new difftool.tabbed feature

[Nicholas Guriev]

And related the `--tabbed` command line option and the
GIT_DIFFTOOL_TABBED environment variable.
---
 Documentation/config/difftool.txt |  6 ++++++
 Documentation/git-difftool.txt    | 19 +++++++++++++++----
 Documentation/git.txt             |  4 ++++
 3 files changed, 25 insertions(+), 4 deletions(-)

diff --git a/Documentation/config/difftool.txt b/Documentation/config/difftool.txt
index 6762594480..ac609aee66 100644
--- a/Documentation/config/difftool.txt
+++ b/Documentation/config/difftool.txt
@@ -12,3 +12,9 @@ difftool.<tool>.cmd::
 
 difftool.prompt::
 	Prompt before each invocation of the diff tool.
+
+difftool.tabbed::
+	Show compared files in different tabs using single invocation of
+	the diff tool. Must be a boolean value. Only the following tools
+	are currently supported: vimdiff and related, meld. Tools with
+	overridden command line will ignore this configuration variable.
diff --git a/Documentation/git-difftool.txt b/Documentation/git-difftool.txt
index 484c485fd0..1b7a5345ad 100644
--- a/Documentation/git-difftool.txt
+++ b/Documentation/git-difftool.txt
@@ -69,6 +69,13 @@ with custom merge tool commands and has the same value as `$MERGED`.
 --tool-help::
 	Print a list of diff tools that may be used with `--tool`.
 
+--[no-]tabbed::
+	Open compared files in different tabs using single invocation
+	of the diff tool. This overrides configuration or environment.
+	Currently, only the following tools are supported: vimdiff and
+	related, meld. Tools with overridden command line will ignore
+	this option.
+
 --[no-]symlinks::
 	'git difftool''s default behavior is create symlinks to the
 	working tree when run in `--dir-diff` mode and the right-hand
@@ -95,10 +102,11 @@ instead.  `--no-symlinks` is the default on Windows.
 	`diff.tool`, `merge.tool` until a tool is found.
 
 --[no-]trust-exit-code::
-	'git-difftool' invokes a diff tool individually on each file.
-	Errors reported by the diff tool are ignored by default.
-	Use `--trust-exit-code` to make 'git-difftool' exit when an
-	invoked diff tool returns a non-zero exit code.
+	'git-difftool' invokes a diff tool individually on each file
+	unless tabbed mode is active. Errors reported by the diff tool
+	are ignored by default. Use `--trust-exit-code` to make
+	'git-difftool' exit immediately when an invoked diff tool
+	returns a non-zero exit code.
 +
 'git-difftool' will forward the exit code of the invoked tool when
 `--trust-exit-code` is used.
@@ -128,6 +136,9 @@ See the `--tool=<tool>` option above for more details.
 difftool.prompt::
 	Prompt before each invocation of the diff tool.
 
+difftool.tabbed::
+	Configure default value of the `--tabbed` option. See above.
+
 difftool.trustExitCode::
 	Exit difftool if the invoked diff tool returns a non-zero exit status.
 +
diff --git a/Documentation/git.txt b/Documentation/git.txt
index a6d4ad0818..d7cd6650c9 100644
--- a/Documentation/git.txt
+++ b/Documentation/git.txt
@@ -583,6 +583,10 @@ For each path `GIT_EXTERNAL_DIFF` is called, two environment variables,
 `GIT_DIFF_PATH_TOTAL`::
 	The total number of paths.
 
+`GIT_DIFFTOOL_TABBED`::
+	Run the diff tool in tabbed mode opening all compared files
+	together. It must contain a boolean value.
+
 other
 ~~~~~
 `GIT_MERGE_VERBOSITY`::
-- 
2.27.0

Re: [RFC PATCH v2 1/3] mergetools: support difftool.tabbed setting

[Junio C Hamano]

Nicholas Guriev <nicholas@guriev.su> writes:

> diff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh
> index 78f3647ed9..2a1edbb9b9 100644
> --- a/git-mergetool--lib.sh
> +++ b/git-mergetool--lib.sh
> @@ -195,6 +195,11 @@ setup_tool () {
>  		false
>  	}
>  
> +	# Clear combo function declared by a previous tool (if any) to
> +	# unambiguously indicate that the current one supports the feature or
> +	# not.
> +	unset -f diff_combo_cmd
> +

I know this is so that you can use "type diff_combo_cmd" in a
different part of the patch, but I do not think you want to be
fooled by an unrelated ~/bin/diff_combo_cmd that the user may have
under the home directory.

Instead, the fallback definitions can have

	diff_combo_supported () {
		return 1
	}

and have backends that does support diff_combo_supported to override
with their own

	diff_combo_supported () {
		return 0
	}

And then, the part of is_difftool_tabbed that wants to see if the
current backend supports the operation can become:

	test "$GIT_DIFFTOOL_TABBED" = true &&
	test "${GIT_DIFF_PATH_TOTAL=0}" -gt 1 &&
-	type diff_combo_cmd >/dev/null 2>&1
+	diff_combo_supported
 }

That way, you do not have to do "unset -f" up above.


> @@ -250,6 +255,28 @@ trust_exit_code () {
>  	fi
>  }
>  
> +# Check whether tabbed mode is requested and current loaded tool supports it.
> +is_difftool_tabbed () {
> +	: "${GIT_DIFFTOOL_TABBED=$(git config --type=bool \
> +		--default=false difftool.tabbed || echo bad value)}"
> +	case $(printf "%s" "$GIT_DIFFTOOL_TABBED" | tr '[:upper:]' '[:lower:]') in
> +	yes|on|true|1)
> +		GIT_DIFFTOOL_TABBED=true
> +		;;
> +	no|off|false|0|'')
> +		GIT_DIFFTOOL_TABBED=false
> +		;;
> +	*)
> +		echo "error: bad boolean value of GIT_DIFFTOOL_TABBED" >&2
> +		exit 1
> +		;;
> +	esac
> +
> +	test "$GIT_DIFFTOOL_TABBED" = true &&
> +	test "${GIT_DIFF_PATH_TOTAL=0}" -gt 1 &&
> +	type diff_combo_cmd >/dev/null 2>&1
> +}

[PATCH v3 0/4] difftools in tabbed mode

[Nicholas Guriev]

Junio C Hamano, thank you for the comment. I added the diff_combo_supported
function as you suggested. Though it calls to the `true` or the `false` builtins
as I find them much clearer than the "return" command with inverted status. It
is really confusing that C and most other programming languages adequately
interpret non-zero as truth value and zero as falsity value unlike Unix Shell.
Are such changes acceptable?

As regards broken tests, it may seem silly but that error was due to a single
missing colon. I corrected the mistake and added some automated tests of this
new feature.

diff --git a/git-difftool--helper.sh b/git-difftool--helper.sh
index 529d55c96d..85a6dc9c30 100755
--- a/git-difftool--helper.sh
+++ b/git-difftool--helper.sh
@@ -31,7 +31,7 @@ prompt_before_launch () {
 	do
 		printf "Launch '%s' [Y/n]? " "${GIT_DIFFTOOL_EXTCMD:-$merge_tool}"
 		read ans 2>/dev/null || return 1
-		case "${ans-y}" in
+		case "${ans:-y}" in
 		[yY]*) return 0 ;;
 		[nN]*) return 1 ;;
 		esac

Changes compared to v2:
 * Introduced the diff_combo_supported function.
 * Fixed default answer to difftool prompting.
 * More docs about the diff tool mechanism.
 * New auto-tests of the feature.

Nicholas Guriev (4):
  mergetools: support difftool.tabbed setting
  difftool-helper: conciliate difftool.tabbed and difftool.prompt
    settings
  doc: describe new difftool.tabbed feature
  t7800: new tests of difftool.tabbed feature

 Documentation/config/difftool.txt    |   6 +
 Documentation/git-difftool.txt       |  19 ++-
 Documentation/git-mergetool--lib.txt |  62 ++++++++++
 Documentation/git.txt                |   8 ++
 builtin/difftool.c                   |   7 +-
 diff.c                               |  10 +-
 git-difftool--helper.sh              |  39 ++++--
 git-mergetool--lib.sh                |  71 ++++++++++-
 mergetools/meld                      |   8 ++
 mergetools/vimdiff                   |  21 ++++
 t/t7800-difftool.sh                  | 175 ++++++++++++++++++++++++++-
 11 files changed, 401 insertions(+), 25 deletions(-)

-- 
2.27.0

[PATCH v3 1/4] mergetools: support difftool.tabbed setting

[Nicholas Guriev]

The patch introduces a new boolean setting, difftool.tabbed, which
allows a user to view diffs in single window at one go. The `--tabbed`
command line option and the GIT_DIFFTOOL_TABBED environment variable are
also available. For now, it works only with vimdiff and related, meld.

We call to a new invented function, diff_combo_cmd, if it is provided by
the diff tool, and pass to it a list of pairs of compared files. The
list is available through third file descriptor, filenames are separated
by a line feed, '\n', so they can be split by means of unmodified $IFS.
The function may close that descriptor after reading the list.

Signed-off-by: Nicholas Guriev <nicholas@guriev.su>
---
 builtin/difftool.c    |  7 +++++-
 diff.c                | 10 ++++++++-
 git-mergetool--lib.sh | 51 ++++++++++++++++++++++++++++++++++++++-----
 mergetools/meld       |  8 +++++++
 mergetools/vimdiff    | 21 ++++++++++++++++++
 5 files changed, 90 insertions(+), 7 deletions(-)

diff --git a/builtin/difftool.c b/builtin/difftool.c
index 6e18e623fd..f061d3c029 100644
--- a/builtin/difftool.c
+++ b/builtin/difftool.c
@@ -689,7 +689,7 @@ static int run_file_diff(int prompt, const char *prefix,
 int cmd_difftool(int argc, const char **argv, const char *prefix)
 {
 	int use_gui_tool = 0, dir_diff = 0, prompt = -1, symlinks = 0,
-	    tool_help = 0, no_index = 0;
+	    tabbed = -1, tool_help = 0, no_index = 0;
 	static char *difftool_cmd = NULL, *extcmd = NULL;
 	struct option builtin_difftool_options[] = {
 		OPT_BOOL('g', "gui", &use_gui_tool,
@@ -708,6 +708,8 @@ int cmd_difftool(int argc, const char **argv, const char *prefix)
 		OPT_BOOL(0, "tool-help", &tool_help,
 			 N_("print a list of diff tools that may be used with "
 			    "`--tool`")),
+		OPT_BOOL_F(0, "tabbed", &tabbed,
+			   N_("use tabs in diff tool if supported"), 0),
 		OPT_BOOL(0, "trust-exit-code", &trust_exit_code,
 			 N_("make 'git-difftool' exit when an invoked diff "
 			    "tool returns a non - zero exit code")),
@@ -756,6 +758,9 @@ int cmd_difftool(int argc, const char **argv, const char *prefix)
 			die(_("no <cmd> given for --extcmd=<cmd>"));
 	}
 
+	if (tabbed >= 0)
+		setenv("GIT_DIFFTOOL_TABBED", tabbed ? "true" : "false", 1);
+
 	setenv("GIT_DIFFTOOL_TRUST_EXIT_CODE",
 	       trust_exit_code ? "true" : "false", 1);
 
diff --git a/diff.c b/diff.c
index 2253ec8802..2868848bba 100644
--- a/diff.c
+++ b/diff.c
@@ -1730,6 +1730,14 @@ static struct diff_tempfile *claim_diff_tempfile(void)
 	BUG("diff is failing to clean up its tempfiles");
 }
 
+static void forget_tempfile(void)
+{
+	for (int i = 0; i < ARRAY_SIZE(diff_temp); i++) {
+		close_tempfile_gently(diff_temp[i].tempfile);
+		diff_temp[i] = (struct diff_tempfile){0};
+	}
+}
+
 static void remove_tempfile(void)
 {
 	int i;
@@ -4275,7 +4283,7 @@ static void run_external_diff(const char *pgm,
 	if (run_command_v_opt_cd_env(argv.v, RUN_USING_SHELL, NULL, env.v))
 		die(_("external diff died, stopping at %s"), name);
 
-	remove_tempfile();
+	forget_tempfile();
 	strvec_clear(&argv);
 	strvec_clear(&env);
 }
diff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh
index 78f3647ed9..fef9289618 100644
--- a/git-mergetool--lib.sh
+++ b/git-mergetool--lib.sh
@@ -151,19 +151,25 @@ setup_tool () {
 
 	# Fallback definitions, to be overridden by tools.
 	can_merge () {
-		return 0
+		true
 	}
 
 	can_diff () {
-		return 0
+		true
 	}
 
 	diff_cmd () {
-		return 1
+		false
+	}
+
+	# The concrete diff tool may define that it supports combined
+	# invocations and the diff_combo_cmd function may be called.
+	diff_combo_supported () {
+		false
 	}
 
 	merge_cmd () {
-		return 1
+		false
 	}
 
 	translate_merge_tool_path () {
@@ -250,6 +256,28 @@ trust_exit_code () {
 	fi
 }
 
+# Check whether tabbed mode is requested and current loaded tool supports it.
+is_difftool_tabbed () {
+	: "${GIT_DIFFTOOL_TABBED=$(git config --type=bool \
+		--default=false difftool.tabbed || echo bad value)}"
+	case $(printf "%s" "$GIT_DIFFTOOL_TABBED" | tr '[:upper:]' '[:lower:]') in
+	yes|on|true|1)
+		GIT_DIFFTOOL_TABBED=true
+		;;
+	no|off|false|0|'')
+		GIT_DIFFTOOL_TABBED=false
+		;;
+	*)
+		echo "error: bad boolean value of GIT_DIFFTOOL_TABBED" >&2
+		exit 1
+		;;
+	esac
+
+	test "$GIT_DIFFTOOL_TABBED" = true &&
+	test "${GIT_DIFF_PATH_TOTAL=0}" -gt 1 &&
+	diff_combo_supported
+}
+
 
 # Entry point for running tools
 run_merge_tool () {
@@ -274,7 +302,20 @@ run_merge_tool () {
 
 # Run a either a configured or built-in diff tool
 run_diff_cmd () {
-	diff_cmd "$1"
+	if is_difftool_tabbed
+	then
+		temp_file="${TMPDIR:-/tmp}/git-${PPID}_tabbed-queue"
+		test "$GIT_DIFF_PATH_COUNTER" -eq 1 && >"$temp_file"
+		printf '%s\n' "$LOCAL" "$REMOTE" >>"$temp_file"
+
+		if test "$GIT_DIFF_PATH_COUNTER" -eq "$GIT_DIFF_PATH_TOTAL"
+		then
+			diff_combo_cmd 3<"$temp_file"
+			rm -f -- "$temp_file"
+		fi
+	else
+		diff_cmd "$1"
+	fi
 }
 
 # Run a either a configured or built-in merge tool
diff --git a/mergetools/meld b/mergetools/meld
index aab4ebb935..e2b9d456c1 100644
--- a/mergetools/meld
+++ b/mergetools/meld
@@ -2,6 +2,14 @@ diff_cmd () {
 	"$merge_tool_path" "$LOCAL" "$REMOTE"
 }
 
+diff_combo_cmd () {
+	"$merge_tool_path" $(printf -- '--diff\n%s\n%s\n' `cat <&3`) 3<&-
+}
+
+diff_combo_supported () {
+	true
+}
+
 merge_cmd () {
 	check_meld_for_features
 
diff --git a/mergetools/vimdiff b/mergetools/vimdiff
index abc8ce4ec4..fe1923076b 100644
--- a/mergetools/vimdiff
+++ b/mergetools/vimdiff
@@ -3,6 +3,27 @@ diff_cmd () {
 		-c 'wincmd l' -c 'cd $GIT_PREFIX' "$LOCAL" "$REMOTE"
 }
 
+multitabbed_script='
+	let i = 1
+	while i < argc()
+		execute "tabedit" fnameescape(argv(i - 1))
+		execute "diffsplit" fnameescape(argv(i))
+		wincmd L
+		let i = i + 2
+	endwhile
+	tabfirst
+	tabclose
+	unlet i
+	chdir $GIT_PREFIX
+'
+diff_combo_cmd () {
+	"$merge_tool_path" -R -f -c "$multitabbed_script" -- `cat <&3` 3<&-
+}
+
+diff_combo_supported () {
+	true
+}
+
 merge_cmd () {
 	case "$1" in
 	*vimdiff)
-- 
2.27.0

[PATCH v3 2/4] difftool-helper: conciliate difftool.tabbed and difftool.prompt settings

[Nicholas Guriev]

The commit combines paths of compared files into a separate temporary
file and prints them before launch the tool on last invocation of the
helper. All temporary files will be removed before exiting after that.
At least, we try. But it may happen the files remain in case of a bug
or a random SIGKILL.

Signed-off-by: Nicholas Guriev <nicholas@guriev.su>
---
 git-difftool--helper.sh | 39 ++++++++++++++++++++++++++-------------
 git-mergetool--lib.sh   | 34 +++++++++++++++++++++++++++-------
 2 files changed, 53 insertions(+), 20 deletions(-)

diff --git a/git-difftool--helper.sh b/git-difftool--helper.sh
index 46af3e60b7..85a6dc9c30 100755
--- a/git-difftool--helper.sh
+++ b/git-difftool--helper.sh
@@ -11,8 +11,8 @@ TOOL_MODE=diff
 # difftool.prompt controls the default prompt/no-prompt behavior
 # and is overridden with $GIT_DIFFTOOL*_PROMPT.
 should_prompt () {
-	prompt_merge=$(git config --bool mergetool.prompt || echo true)
-	prompt=$(git config --bool difftool.prompt || echo $prompt_merge)
+	prompt=$(git config --bool mergetool.prompt || echo true)
+	prompt=$(git config --bool difftool.prompt || echo $prompt)
 	if test "$prompt" = true
 	then
 		test -z "$GIT_DIFFTOOL_NO_PROMPT"
@@ -26,6 +26,18 @@ use_ext_cmd () {
 	test -n "$GIT_DIFFTOOL_EXTCMD"
 }
 
+prompt_before_launch () {
+	while true
+	do
+		printf "Launch '%s' [Y/n]? " "${GIT_DIFFTOOL_EXTCMD:-$merge_tool}"
+		read ans 2>/dev/null || return 1
+		case "${ans:-y}" in
+		[yY]*) return 0 ;;
+		[nN]*) return 1 ;;
+		esac
+	done
+}
+
 launch_merge_tool () {
 	# Merged is the filename as it appears in the work tree
 	# Local is the contents of a/filename
@@ -40,19 +52,20 @@ launch_merge_tool () {
 	# the user with the real $MERGED name before launching $merge_tool.
 	if should_prompt
 	then
-		printf "\nViewing (%s/%s): '%s'\n" "$GIT_DIFF_PATH_COUNTER" \
-			"$GIT_DIFF_PATH_TOTAL" "$MERGED"
-		if use_ext_cmd
+		append_templist merged-paths "$MERGED"
+		# Do preinit before test whether the tool supports tabbed run.
+		use_ext_cmd || setup_tool "$merge_tool"
+
+		if ! is_difftool_tabbed
 		then
-			printf "Launch '%s' [Y/n]? " \
-				"$GIT_DIFFTOOL_EXTCMD"
-		else
-			printf "Launch '%s' [Y/n]? " "$merge_tool"
-		fi
-		read ans || return
-		if test "$ans" = n
+			printf "\nViewing (%d/%d): '%s'\n" "$GIT_DIFF_PATH_COUNTER" \
+				"$GIT_DIFF_PATH_TOTAL" "$MERGED"
+			prompt_before_launch || return
+		elif on_last_file
 		then
-			return
+			printf "Viewing %d files:\n" "$GIT_DIFF_PATH_TOTAL"
+			printf "  '%s'\n" `cat "$LAST_TEMPFILE"`
+			prompt_before_launch || return
 		fi
 	fi
 
diff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh
index fef9289618..c29abfa4fb 100644
--- a/git-mergetool--lib.sh
+++ b/git-mergetool--lib.sh
@@ -22,6 +22,10 @@ is_available () {
 	type "$merge_tool_path" >/dev/null 2>&1
 }
 
+on_last_file () {
+	test "$GIT_DIFF_PATH_COUNTER" -eq "$GIT_DIFF_PATH_TOTAL" 2>/dev/null
+}
+
 list_config_tools () {
 	section=$1
 	line_prefix=${2:-}
@@ -278,6 +282,26 @@ is_difftool_tabbed () {
 	diff_combo_supported
 }
 
+# Save lines to the chosen temporary file for usage from subsequent invocations.
+# Path to the file will be placed to the LAST_TEMPFILE variable for later links.
+append_templist () {
+	LAST_TEMPFILE="${TMPDIR:-/tmp}/git-${PPID}_$1"
+	shift
+
+	if test "$GIT_DIFF_PATH_COUNTER" -eq 1
+	then
+		printf '%s\n' "$@" >"$LAST_TEMPFILE"
+	else
+		printf '%s\n' "$@" >>"$LAST_TEMPFILE"
+	fi
+}
+
+# Clean any temporary files that may create this script.
+clean_templists () {
+	rm -f -- "${TMPDIR:-/tmp}/git-${PPID}"_*
+}
+trap 'on_last_file && clean_templists' EXIT INT TERM
+
 
 # Entry point for running tools
 run_merge_tool () {
@@ -304,14 +328,10 @@ run_merge_tool () {
 run_diff_cmd () {
 	if is_difftool_tabbed
 	then
-		temp_file="${TMPDIR:-/tmp}/git-${PPID}_tabbed-queue"
-		test "$GIT_DIFF_PATH_COUNTER" -eq 1 && >"$temp_file"
-		printf '%s\n' "$LOCAL" "$REMOTE" >>"$temp_file"
-
-		if test "$GIT_DIFF_PATH_COUNTER" -eq "$GIT_DIFF_PATH_TOTAL"
+		append_templist tabbed-queue "$LOCAL" "$REMOTE"
+		if on_last_file
 		then
-			diff_combo_cmd 3<"$temp_file"
-			rm -f -- "$temp_file"
+			diff_combo_cmd 3<"$LAST_TEMPFILE"
 		fi
 	else
 		diff_cmd "$1"
-- 
2.27.0

[PATCH v3 3/4] doc: describe new difftool.tabbed feature

[Nicholas Guriev]

And related the `--tabbed` command line option, and the
GIT_DIFFTOOL_TABBED environment variable.

Signed-off-by: Nicholas Guriev <nicholas@guriev.su>
---
 Documentation/config/difftool.txt    |  6 +++
 Documentation/git-difftool.txt       | 19 +++++++--
 Documentation/git-mergetool--lib.txt | 62 ++++++++++++++++++++++++++++
 Documentation/git.txt                |  8 ++++
 4 files changed, 91 insertions(+), 4 deletions(-)

diff --git a/Documentation/config/difftool.txt b/Documentation/config/difftool.txt
index 6762594480..ac609aee66 100644
--- a/Documentation/config/difftool.txt
+++ b/Documentation/config/difftool.txt
@@ -12,3 +12,9 @@ difftool.<tool>.cmd::
 
 difftool.prompt::
 	Prompt before each invocation of the diff tool.
+
+difftool.tabbed::
+	Show compared files in different tabs using single invocation of
+	the diff tool. Must be a boolean value. Only the following tools
+	are currently supported: vimdiff and related, meld. Tools with
+	overridden command line will ignore this configuration variable.
diff --git a/Documentation/git-difftool.txt b/Documentation/git-difftool.txt
index 484c485fd0..1b7a5345ad 100644
--- a/Documentation/git-difftool.txt
+++ b/Documentation/git-difftool.txt
@@ -69,6 +69,13 @@ with custom merge tool commands and has the same value as `$MERGED`.
 --tool-help::
 	Print a list of diff tools that may be used with `--tool`.
 
+--[no-]tabbed::
+	Open compared files in different tabs using single invocation
+	of the diff tool. This overrides configuration or environment.
+	Currently, only the following tools are supported: vimdiff and
+	related, meld. Tools with overridden command line will ignore
+	this option.
+
 --[no-]symlinks::
 	'git difftool''s default behavior is create symlinks to the
 	working tree when run in `--dir-diff` mode and the right-hand
@@ -95,10 +102,11 @@ instead.  `--no-symlinks` is the default on Windows.
 	`diff.tool`, `merge.tool` until a tool is found.
 
 --[no-]trust-exit-code::
-	'git-difftool' invokes a diff tool individually on each file.
-	Errors reported by the diff tool are ignored by default.
-	Use `--trust-exit-code` to make 'git-difftool' exit when an
-	invoked diff tool returns a non-zero exit code.
+	'git-difftool' invokes a diff tool individually on each file
+	unless tabbed mode is active. Errors reported by the diff tool
+	are ignored by default. Use `--trust-exit-code` to make
+	'git-difftool' exit immediately when an invoked diff tool
+	returns a non-zero exit code.
 +
 'git-difftool' will forward the exit code of the invoked tool when
 `--trust-exit-code` is used.
@@ -128,6 +136,9 @@ See the `--tool=<tool>` option above for more details.
 difftool.prompt::
 	Prompt before each invocation of the diff tool.
 
+difftool.tabbed::
+	Configure default value of the `--tabbed` option. See above.
+
 difftool.trustExitCode::
 	Exit difftool if the invoked diff tool returns a non-zero exit status.
 +
diff --git a/Documentation/git-mergetool--lib.txt b/Documentation/git-mergetool--lib.txt
index 4da9d24096..1b9fb3591e 100644
--- a/Documentation/git-mergetool--lib.txt
+++ b/Documentation/git-mergetool--lib.txt
@@ -44,6 +44,68 @@ run_merge_tool::
 	'$MERGED', '$LOCAL', '$REMOTE', and '$BASE' must be defined
 	for use by the merge tool.
 
+TOOLS
+-----
+
+There are several built-in merge tool wrappers which are located in the
+'$(git --exec-path)/mergetools' directory. They are shell scripts and provide
+a unified interface for the discussed scriptlet. It expects to find the
+following functions defined by the each wrapper. However, most of them have
+sane default implementation and the wrapper may write less boilerplate.
+
+can_merge::
+	returns zero status (true) if the tool can be used by `git mergetool`,
+	otherwise the command will be unavailable. Default: true.
+
+can_diff::
+	returns zero status (true) if the tool can be used by `git difftool`,
+	otherwise the command will be unavailable. Default: true.
+
+merge_cmd::
+	should actually launch the tool in merging mode for a single path.
+	Positional argument: '$1' -- name of the merge tool. Predefined
+	variables: '$MERGED', '$LOCAL', '$REMOTE', and '$BASE' mentioned above,
+	'$merge_tool_path' -- absolute path to the binary of the tool or its name
+	if seen in default search path, '$base_present' -- string `true` or `false`.
+
+diff_cmd::
+	should actually launch the tool in diffing mode for a single path.
+	Positional argument: '$1' -- name of the diff tool. Predefined
+	variables: '$LOCAL' and '$REMOTE', '$merge_tool_path' -- absolute path
+	to the binary of the tool or its name if seen in default search path.
+
+diff_combo_supported::
+	returns zero status (true) if the tool can operate in tabbed mode,
+	otherwise the feature will be skipped. Default: false.
+
+diff_combo_cmd::
+	should actually launch the tool with all compared files. The function
+	receives list of pairs of files to compare, every filename is followed
+	by proper input field separator, '$IFS', the list is reachable through
+	third opened file descriptor and the function may close this fd after
+	reading all its content. System file descriptors remain untouched.
+	Predefined variable: '$merge_tool_path' -- absolute path to the binary
+	of the tool or its name if seen in default search path.
++
+Example:
++
+[listing]
+diff_combo_cmd () {
+	"$merge_tool_path" -- `cat <&3` 3<&-
+}
+
+translate_merge_tool_path::
+	can print preset '$merge_tool_path'. Positional argument: '$1' -- name
+	of the merge tool.
+
+exit_code_trustable::
+	returns zero status (true) if a caller is allowed to rely on the exit
+	code of the merge tool, otherwise one should check a backup of the file
+	to determine whether or not merging was successful. Default: false.
+
+list_tool_variants::
+	prints all possible names that may be passed to the '--tool' option.
+
 GIT
 ---
 Part of the linkgit:git[1] suite
diff --git a/Documentation/git.txt b/Documentation/git.txt
index a6d4ad0818..3ef75aae36 100644
--- a/Documentation/git.txt
+++ b/Documentation/git.txt
@@ -583,6 +583,14 @@ For each path `GIT_EXTERNAL_DIFF` is called, two environment variables,
 `GIT_DIFF_PATH_TOTAL`::
 	The total number of paths.
 
+`GIT_DIFFTOOL_PROMPT`::
+	Issue an interactive prompting right before launch the diff
+	tool. It must contain a boolean value.
+
+`GIT_DIFFTOOL_TABBED`::
+	Run the diff tool in tabbed mode opening all compared files
+	together. It must contain a boolean value.
+
 other
 ~~~~~
 `GIT_MERGE_VERBOSITY`::
-- 
2.27.0

[PATCH v3 4/4] t7800: new tests of difftool.tabbed feature

[Nicholas Guriev]

A few helper functions were put in the script that manage faked binaries in a
temporary directory for $PATH. The helpers restore state before test finish.

Besides, two tests of rewritten prompting were added. "git difftool" now asks
again on incorrect input. Plus, fixed a typo in a test of the --extcmd option.

Signed-off-by: Nicholas Guriev <nicholas@guriev.su>
---
 t/t7800-difftool.sh | 175 +++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 174 insertions(+), 1 deletion(-)

diff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh
index a578b35761..249d7a4821 100755
--- a/t/t7800-difftool.sh
+++ b/t/t7800-difftool.sh
@@ -270,6 +270,22 @@ test_expect_success 'difftool last flag wins' '
 	prompt_given "$prompt"
 '
 
+test_expect_success 'ignore unknown input then launch the tool' '
+	difftool_test_setup &&
+	(echo Qwerty && echo Yes) >input &&
+	git difftool branch <input >output &&
+	tail -1 output | grep -q -x -F \
+		"Launch '\''test-tool'\'' [Y/n]? Launch '\''test-tool'\'' [Y/n]? branch"
+'
+
+test_expect_success 'ignore unknown input then skip the tool' '
+	difftool_test_setup &&
+	(echo Qwerty && echo No) >input &&
+	git difftool branch <input >output &&
+	tail -1 output | grep -q -x -F \
+		"Launch '\''test-tool'\'' [Y/n]? Launch '\''test-tool'\'' [Y/n]? "
+'
+
 # git-difftool falls back to git-mergetool config variables
 # so test that behavior here
 test_expect_success 'difftool + mergetool config variables' '
@@ -319,7 +335,7 @@ test_expect_success 'difftool --extcmd=cat' '
 test_expect_success 'difftool --extcmd cat' '
 	echo branch >expect &&
 	echo master >>expect &&
-	git difftool --no-prompt --extcmd=cat branch >actual &&
+	git difftool --no-prompt --extcmd cat branch >actual &&
 	test_cmp expect actual
 '
 
@@ -390,6 +406,163 @@ test_expect_success 'ending prompt input with EOF' '
 	! grep br2 output
 '
 
+prepend_shared_path () {
+	directory="$1"
+	first="${PATH%%:*}"
+	if test "$directory" != "$first"
+	then
+		: ${clean_shared_path=$PATH}
+		PATH="$directory:$PATH"
+		test_when_finished 'PATH=$clean_shared_path'
+	fi
+}
+
+mock_binary () {
+	name="$1" body="$2"
+	prepend_shared_path "$PWD/fake-bin" &&
+	touch run-cmds &&
+	mkdir -p fake-bin &&
+	write_script "fake-bin/$name" <<-EOF &&
+	printf '%s ' $name "\$@" |
+	tr -s '[:space:]' ' ' |
+	sed 's/ \$/\\n/' >>"\$HOME/run-cmds" &&
+	${body:-true}
+	EOF
+	test_when_finished "rm -f fake-bin/$name run-cmds"
+}
+
+test_invoked () {
+	pattern="$1"
+	if ! grep -q -x -e "$pattern" run-cmds
+	then
+		set -- $pattern
+		echo "$1 does not seem to be invoked"
+		echo "entry '$pattern' was not found in the 'run-cmds' file"
+		false
+	fi
+}
+
+test_expect_success 'tabbed mode with --tabbed option' '
+	mock_binary vim &&
+	test_config_global diff.tool vimdiff &&
+	yes | git difftool --tabbed branch | tee actual &&
+	(
+		printf "Viewing 2 files:\n" &&
+		printf "  '\''file'\''\n" &&
+		printf "  '\''file2'\''\n" &&
+		printf "Launch '\''vimdiff'\'' [Y/n]? "
+	) >expect &&
+	test_cmp expect actual &&
+	test_invoked "vim -R -f -c .* file .* file2"
+'
+
+test_expect_success 'tabbed mode with GIT_DIFFTOOL_TABBED environment' '
+	mock_binary vim &&
+	test_config_global diff.tool vimdiff &&
+	yes | GIT_DIFFTOOL_TABBED=yes git difftool branch | tee actual &&
+	(
+		printf "Viewing 2 files:\n" &&
+		printf "  '\''file'\''\n" &&
+		printf "  '\''file2'\''\n" &&
+		printf "Launch '\''vimdiff'\'' [Y/n]? "
+	) >expect &&
+	test_cmp expect actual &&
+	test_invoked "vim -R -f -c .* file .* file2"
+'
+
+test_expect_success 'tabbed mode with difftool.tabbed setting' '
+	mock_binary vim &&
+	test_config_global diff.tool vimdiff &&
+	test_config_global difftool.tabbed 1 &&
+	yes | git difftool branch | tee actual &&
+	(
+		printf "Viewing 2 files:\n" &&
+		printf "  '\''file'\''\n" &&
+		printf "  '\''file2'\''\n" &&
+		printf "Launch '\''vimdiff'\'' [Y/n]? "
+	) >expect &&
+	test_cmp expect actual &&
+	test_invoked "vim -R -f -c .* file .* file2"
+'
+
+test_expect_success 'environment variable wins over config in tabbed mode' '
+	mock_binary vim &&
+	test_config_global diff.tool vimdiff &&
+	test_config_global difftool.tabbed true &&
+	GIT_DIFFTOOL_TABBED=FALSE git difftool -y branch </dev/null >output &&
+	test_must_be_empty output &&
+	test_invoked "vim -R -f -d -c .* file" &&
+	test_invoked "vim -R -f -d -c .* file2"
+'
+
+test_expect_success 'cli option wins over environment in tabbed mode' '
+	mock_binary vim &&
+	test_config_global diff.tool vimdiff &&
+	GIT_DIFFTOOL_TABBED=1 git difftool -y --no-tabbed branch </dev/null >output &&
+	test_must_be_empty output &&
+	test_invoked "vim -R -f -d -c .* file" &&
+	test_invoked "vim -R -f -d -c .* file2"
+'
+
+test_expect_success 'say no in tabbed mode' '
+	mock_binary meld &&
+	yes no | git difftool -t meld --tabbed branch &&
+	! test_invoked "meld\\>.*"
+'
+
+test_expect_success 'no tabbed mode for single file' '
+	mock_binary meld &&
+	git difftool -y -t meld --tabbed branch file &&
+	test_invoked "meld \\S\\+ file"
+'
+
+test_expect_success 'both --tabbed and --trust-exit-code options' '
+	mock_binary meld false &&
+	test_config_global diff.tool meld &&
+	test_config_global difftool.prompt false &&
+	test_must_fail git difftool --tabbed --trust-exit-code branch >output &&
+	test_must_be_empty output &&
+	test_invoked "meld --diff \\S\\+ file --diff \\S\\+ file2"
+'
+
+test_expect_success 'tempdir is still clean after successful tabbed mode' '
+	mock_binary meld &&
+	mkdir tempdir &&
+	test_when_finished "rm -r tempdir" &&
+	TMPDIR="$PWD/tempdir" git difftool -y -t meld --tabbed branch &&
+	test_dir_is_empty tempdir
+'
+
+test_expect_success 'tempdir is still clean after failed tabbed mode' '
+	mock_binary meld false &&
+	mkdir tempdir &&
+	test_when_finished "rm -r tempdir" &&
+	TMPDIR="$PWD/tempdir" git difftool -y -t meld --tabbed branch &&
+	test_dir_is_empty tempdir
+'
+
+test_background () {
+	# https://stackoverflow.com/a/45112755/5000805
+	set -m
+	"$@" &
+	set +m
+}
+
+# Create a named queue for synchronizing. Our test process will get blocked on
+# "echo line" until faked meld reaches "cat chain", and so we do not kill early.
+test_expect_success PIPE 'tempdir is still clean after SIGTERM in tabbed mode' '
+	mkfifo chan &&
+	mock_binary meld "while true; do cat chan; done" &&
+	mkdir tempdir &&
+	test_when_finished "rm -r tempdir" &&
+	test_background env TMPDIR="$PWD/tempdir" \
+		git difftool -y -t meld --tabbed branch &&
+	echo line >chan &&
+	kill -TERM -$! &&
+	wait && # for clean up
+	test_dir_is_empty tempdir
+'
+
 test_expect_success 'difftool --tool-help' '
 	git difftool --tool-help >output &&
 	grep tool output
-- 
2.27.0

Re: [PATCH v3 2/4] difftool-helper: conciliate difftool.tabbed and difftool.prompt settings

[Junio C Hamano]

Nicholas Guriev <nicholas@guriev.su> writes:

> The commit combines paths of compared files into a separate temporary
> file and prints them before launch the tool on last invocation of the
> helper. All temporary files will be removed before exiting after that.
> At least, we try. But it may happen the files remain in case of a bug
> or a random SIGKILL.
>
> Signed-off-by: Nicholas Guriev <nicholas@guriev.su>
> ---

Isn't the introduction of prompt_before_launch etc. would be a
welcome change before step [v3 1/4]?  If it makes a pure improvement
of the existing code without adding any new/additional feature like
the "tabbed" stuff, it would be better be discussed before anything
else in the series.

Then the new "tabbed" stuff can take advantage of the cleaned up
code with the new helpers---the patch that introduces the feature
will become more concise and easier to follow, I would think.

If I am reading the patches right, [v3 1&2/4] goes the other way
around: "let's add a new feature first in an ad-hoc way.  ah, it
seems that the code added for the new feature can be simplified if
we tweak a few things, and while we are doing so, we can adjust old
feature to use the same simplification again".

Regarding what [v3 1/4] does (which if we follow thru the above
suggestion would become [v4 2/4]), I am not sure that we want to add
'if tabbed mode, do this, otherwise do that' to run_diff_cmd.  It
would become awkward to keep piling such a change on top in the
function in the longer run.  I am wondering if each mergetool can
override run_diff_cmd as a whole, when it is dot-sourced to set up
itself.  The logic you added inside is_difftool_tabbed in [v3 1/4]
would probably need to be made available to the dot-sourced backends
so that they can just call something without reimplementing the same
logic, but by doing so, diff_combo_supported would become unneeded.
Tools that do not support the "combo" mode just do not have to do
anything.

But such a change would become easier to review if we do restructure
like this [v3 2/4] does earlier in the series _before_ a new feature
gets introduced.

Thanks.

Re: [RFC PATCH] mergetools: support difftool.tabbed setting

[David Aguilar]

On Tue, Jan 12, 2021 at 10:07 PM Nicholas Guriev <nicholas@guriev.su> wrote:
>
> I was asked how to configure "git difftool" to open files using several
> tabs and stop spawning diff application on every modified file. I looked
> into Git source and found no possibility to run diff tool at one step.
>
> The patch allows a user to view diffs in single window at one go. The
> current implementation is still poor and it can be used solely for
> demonstration purposes. To see it in action, tweak the local gitconfig:
>
>     git config difftool.prompt false
>     git config difftool.tabbed true
>
> Then run:
>
>     git difftool -t vimdiff
>
> Or:
>
>     git difftool -t meld
>
> The solution has some restrictions, diffing up to ten files works now (I
> did not bother with dynamic memory allocation), and it does not handle spaces
> in file names (I do not know how to pass them correctly to underlying tools
> without "xargs -0").
>
> I think the git-difftool--helper should be changed so that it could
> process many files in single invocation and it would not use a temporary
> file by itself. A similar behaviour can be done in git-mergetool, too.
>
> Do you have ideas how to better implement such a feature? Any comments
> are welcome.
>
> P.S.: I'm attaching screenshots for a clear demo what I mean.
> ---
>  diff.c                |  4 ++--
>  git-mergetool--lib.sh | 36 +++++++++++++++++++++++++++++++++++-
>  mergetools/meld       |  4 ++++
>  mergetools/vimdiff    | 17 +++++++++++++++++
>  4 files changed, 58 insertions(+), 3 deletions(-)
>
>
>

I'm not really sure if "tabbed" is the best name for what's going on,
though.  It's really more of a "diff everything in one shot" mode, and
it just so happens that the tools in question use tabs.

General note -- similar to the convention followed by
mergetool.hideResolved and other difftool things I think it would make
sense for tools to be able to override this on a per-tool basis.

That said, I wonder whether we need this new feature, or whether we
should instead improve an existing one.  I'm leaning towards improving
the existing dir-diff feature as a better alternative.

It's unfortunate that the "git difftool --dir-diff" feature doesn't
seem to mesh well with vimdiff.  It does work well with other tools
that support diffing arbitrary directories, notably meld, xxdiff, etc.

Regarding vimdiff + git difftool -d, there is this advice:
https://stackoverflow.com/questions/8156493/git-vimdiff-and-dirdiff

meld works just fine with "git difftool -d" (arguably nicer, since it
gives you a directory view and a diff view in separate tabs), so if
the only improvement is for vimdiff, then maybe the advice with the
DirDiff plugin might be a better way to go.

Having something like a "difftool.vimdiff.useDirDiff" configuration
variable could be a way for us to adopt the advice that they offer
there.  We could have the dir-diff difftool mode set a variable that
the vimdiff scriptlet could use to detect that we're in dir-diff mode.
Then, when that variable is set, vimdiff could use,

    vim -f '+next' '+execute \"DirDiff\" argv(0) argv(1)' $LOCAL $REMOTE

(as mentioned in the SO page) to invoke dir-diff mode in vim.

That way the user only needs to set:

git config difftool.vimdiff.useDirDiff

... and then "git diffftool -d" will integrate with the DirDiff plugin.

What do you think about improving the vimdiff scriptlet to better
integrate with "git difftool -d" instead?
-- 
David

Re: [RFC PATCH] mergetools: support difftool.tabbed setting

[Junio C Hamano]

David Aguilar <davvid@gmail.com> writes:

> I'm not really sure if "tabbed" is the best name for what's going on,
> though.  It's really more of a "diff everything in one shot" mode, and
> it just so happens that the tools in question use tabs.

This statement matches my reaction to this new feature exactly.

The way the external commands are triggered via GIT_EXTERNAL_DIFF
mechanism makes it "easy" to show changes for one path at a time and
"hard" to do so for all paths at once, but the resulting end-user
experience that is forced to view one path at a time may be awkward.

> That said, I wonder whether we need this new feature, or whether we
> should instead improve an existing one.  I'm leaning towards improving
> the existing dir-diff feature as a better alternative. ...

As a non-user, I have no strong opinion on the "new feature"; other
than that I trust your judgement on the "difftool" design issues,
that is.

Thanks.