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

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

[Tim Henigan]

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

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

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

[Tim Henigan]

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(+)

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

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

[Junio C Hamano]

Tim Henigan <tim.henigan@gmail.com> writes:

> 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?

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

[Junio C Hamano]

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?