threads / patch / 30087

patch, 2 partsmergetools: split config files for vim and gvim

Subject: [PATCH 1/2] mergetools: split config files for vim and gvim

## tl;dr

4 messages between Mar 28, 2012 and Mar 28, 2012. Diffs are folded; open one to read it.

replies: 3people: 2as markdown or json

Tim Henigan· Mar 28, 2012, 19:58 UTC · lore

In ae69fd0 (mergetool-lib: combine vimdiff and gvimdiff run blocks), the config files for these two tools were combined since they were nearly identical. This remains true, but having a single config file for both makes it difficult to test that each is installed and capable of running.

This commit splits the two config files. Now 'vim' and 'gvim'
follow the pattern used by all the other diff/merge tools:
  - There is a single file per tool
  - To use the tool, 'diff.tool' (or other similar option) may be
    set to the exact name of the file.
Signed-off-by: Tim Henigan <tim.henigan@gmail.com>
---

This series should apply cleanly on the current master, but it was developed on top of the series that implements the 'difftool --dir-diff' option (currently th/difftool-diffall branched from pu).

This patch does some cleanup needed to be compatible with the new '--dir-diff' option for 'difftool'.

One side-effect of this change which may be a problem is that we lose support for the 'vimdiff2' and 'gvimdiff2' tools that were created in 0008669 (mergetool-lib: make the three-way diff the default for vim/gvim). The 2-panel options were not advertised in any way, so I don't know if it is important to keep them.

 git-mergetool--lib.sh |    9 +--------
 mergetools/gvim       |   21 +++++++++++++++++++++
 mergetools/vim        |   41 +++++++++--------------------------------
 3 files changed, 31 insertions(+), 40 deletions(-)
 create mode 100644 mergetools/gvim
Show changes to 3 files +31 −40

git-mergetool--lib.sh, mergetools/gvim, mergetools/vim

diff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh
index ed630b2..89b16dc 100644
--- a/git-mergetool--lib.sh
+++ b/git-mergetool--lib.sh
@@ -44,14 +44,7 @@ valid_tool () {
 }
 
 setup_tool () {
-	case "$1" in
-	vim*|gvim*)
-		tool=vim
-		;;
-	*)
-		tool="$1"
-		;;
-	esac
+	tool="$1"
 	mergetools="$(git --exec-path)/mergetools"
 
 	# Load the default definitions
diff --git a/mergetools/gvim b/mergetools/gvim
new file mode 100644
index 0000000..b746e6f
--- /dev/null
+++ b/mergetools/gvim
@@ -0,0 +1,21 @@
+diff_cmd () {
+	"$merge_tool_path" -R -f -d \
+		-c 'wincmd l' -c 'cd $GIT_PREFIX' "$LOCAL" "$REMOTE"
+}
+
+merge_cmd () {
+	touch "$BACKUP"
+	if $base_present
+	then
+		"$merge_tool_path" -f -d -c 'wincmd J' \
+			"$MERGED" "$LOCAL" "$BASE" "$REMOTE"
+	else
+		"$merge_tool_path" -f -d -c 'wincmd l' \
+			"$LOCAL" "$MERGED" "$REMOTE"
+	fi
+	check_unchanged
+}
+
+translate_merge_tool_path() {
+	echo gvim
+}
diff --git a/mergetools/vim b/mergetools/vim
index 619594a..6817708 100644
--- a/mergetools/vim
+++ b/mergetools/vim
@@ -1,44 +1,21 @@
 diff_cmd () {
-	case "$1" in
-	gvimdiff|vimdiff)
-		"$merge_tool_path" -R -f -d \
-			-c 'wincmd l' -c 'cd $GIT_PREFIX' "$LOCAL" "$REMOTE"
-		;;
-	gvimdiff2|vimdiff2)
-		"$merge_tool_path" -R -f -d \
-			-c 'wincmd l' -c 'cd $GIT_PREFIX' "$LOCAL" "$REMOTE"
-		;;
-	esac
+	"$merge_tool_path" -R -f -d \
+		-c 'wincmd l' -c 'cd $GIT_PREFIX' "$LOCAL" "$REMOTE"
 }
 
 merge_cmd () {
 	touch "$BACKUP"
-	case "$1" in
-	gvimdiff|vimdiff)
-		if $base_present
-		then
-			"$merge_tool_path" -f -d -c 'wincmd J' \
-				"$MERGED" "$LOCAL" "$BASE" "$REMOTE"
-		else
-			"$merge_tool_path" -f -d -c 'wincmd l' \
-				"$LOCAL" "$MERGED" "$REMOTE"
-		fi
-		;;
-	gvimdiff2|vimdiff2)
+	if $base_present
+	then
+		"$merge_tool_path" -f -d -c 'wincmd J' \
+			"$MERGED" "$LOCAL" "$BASE" "$REMOTE"
+	else
 		"$merge_tool_path" -f -d -c 'wincmd l' \
 			"$LOCAL" "$MERGED" "$REMOTE"
-		;;
-	esac
+	fi
 	check_unchanged
 }
 
 translate_merge_tool_path() {
-	case "$1" in
-	gvimdiff|gvimdiff2)
-		echo gvim
-		;;
-	vimdiff|vimdiff2)
-		echo vim
-		;;
-	esac
+	echo vim
 }
-- 
1.7.10.rc2.21.g8cb1a
Tim Henigan· Mar 28, 2012, 19:58 UTC · re: Tim Henigan · lore

[PATCH 2/2] mergetools: fail if display needed but not present

Prior to this commit, if 'git mergetool' or 'git difftool' were run in a terminal-only session, they might still try to open a tool that required a windowed environment.

This commit teaches 'git-mergetool--lib.sh' to test for the presence of a display prior to opening tools that require one.

NOTE: The DISPLAY variable is not set in msysgit or cygwin Git.  In the
absence of that variable, the script assumes that a window environment
is always available when running msysgit or cygwin.
Signed-off-by: Tim Henigan <tim.henigan@gmail.com>
---

Only 2 of the current diff/merge tools (vim and emerge) can operate without a display.

The assumption that a display is always available on msys or cygwin is probably not optimal. However, I was not able to find a better work- around. It also matches the previous behavior that did not test for the DISPLAY prior to opening the tool.

 git-mergetool--lib.sh |   23 +++++++++++++++++++++++
 mergetools/defaults   |    4 ++++
 mergetools/emerge     |    4 ++++
 mergetools/vim        |    4 ++++
 4 files changed, 35 insertions(+)
Show changes to 4 files +35 −0

git-mergetool--lib.sh, mergetools/defaults, mergetools/emerge, mergetools/vim

diff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh
index 89b16dc..75c8b11 100644
--- a/git-mergetool--lib.sh
+++ b/git-mergetool--lib.sh
@@ -12,6 +12,19 @@ translate_merge_tool_path () {
 	echo "$1"
 }
 
+display_available() {
+	if test -n "$DISPLAY"
+	then
+		return 0
+	fi
+
+	case "$(uname -s)" in
+	MINGW*|CYGWIN*) return 0; break;;
+	esac
+
+	return 1
+}
+
 check_unchanged () {
 	if test "$MERGED" -nt "$BACKUP"
 	then
@@ -190,6 +203,16 @@ get_merge_tool_path () {
 			 "'$merge_tool_path'"
 		exit 1
 	fi
+	if ! display_available
+	then
+		if tool_requires_display
+		then
+			echo >&2 "The $TOOL_MODE tool $merge_tool cannot"\
+			"be run from a terminal-only session.  A window"\
+			"environment is required."
+			exit 1
+		fi
+	fi
 	echo "$merge_tool_path"
 }
 
diff --git a/mergetools/defaults b/mergetools/defaults
index 1d8f2a3..f150227 100644
--- a/mergetools/defaults
+++ b/mergetools/defaults
@@ -7,6 +7,10 @@ can_diff () {
 	return 0
 }
 
+tool_requires_display() {
+	return 0
+}
+
 diff_cmd () {
 	merge_tool_cmd="$(get_merge_tool_cmd "$1")"
 	if test -z "$merge_tool_cmd"
diff --git a/mergetools/emerge b/mergetools/emerge
index f96d9e5..c7eb80b 100644
--- a/mergetools/emerge
+++ b/mergetools/emerge
@@ -1,3 +1,7 @@
+tool_requires_display () {
+	return 1
+}
+
 diff_cmd () {
 	"$merge_tool_path" -f emerge-files-command "$LOCAL" "$REMOTE"
 }
diff --git a/mergetools/vim b/mergetools/vim
index 6817708..ffd9fd6 100644
--- a/mergetools/vim
+++ b/mergetools/vim
@@ -1,3 +1,7 @@
+tool_requires_display () {
+	return 1
+}
+
 diff_cmd () {
 	"$merge_tool_path" -R -f -d \
 		-c 'wincmd l' -c 'cd $GIT_PREFIX' "$LOCAL" "$REMOTE"
-- 
1.7.10.rc2.21.g8cb1a
Junio C Hamano· Mar 28, 2012, 21:02 UTC · re: Tim Henigan · lore

Re: [PATCH 2/2] mergetools: fail if display needed but not present

Tim Henigan <tim.henigan@gmail.com> writes:
> Prior to this commit, if 'git mergetool' or 'git difftool' were run in a
> terminal-only session, they might still try to open a tool that required
> a windowed environment.

When you say "The command does X. X is bad for such and such reasons. Make it do Y instead, because it is nicer for such and such reasons.", everybody would understand that the command does X without your patch. Maybe it is just me, but I find the phrases like "Prior to this commit" or "Currently" somewhat irritating.

> This commit teaches 'git-mergetool--lib.sh' to test for the presence of
> a display prior to opening tools that require one.

Hrm, why not make it more general, so that mergetool--lib does not have to know anything about DISPLAY but allow the tool scripts to inspect their environment and make their own decision, i.e. after loading the tool scriptlet, the caller can call "can_run [--quiet]" and the scriptlet can either return 0/1 and optionally issue the help message?

Junio C Hamano· Mar 28, 2012, 20:53 UTC · re: Tim Henigan · lore

Re: [PATCH 1/2] mergetools: split config files for vim and gvim

Tim Henigan <tim.henigan@gmail.com> writes:
Show 5 quoted lines
> One side-effect of this change which may be a problem is that we
> lose support for the 'vimdiff2' and 'gvimdiff2' tools that were
> created in 0008669 (mergetool-lib: make the three-way diff the
> default for vim/gvim).  The 2-panel options were not advertised in
> any way, so I don't know if it is important to keep them.

By default, anything we support is important unless there is a sound justification to say otherwise. I think they were kept for people who were already used to the 2-pane version when 3-pane one was introduced.

But I will not be a good judge for this particular case, as I do not use vim nor gvim for merge resolution. Davidd?

← back to recent threads