threads / patch / 28151

patch, 4 partsRefactor git-mergetool--lib.sh

Subject: [PATCH 0/4] Refactor git-mergetool--lib.sh

## tl;dr

18 messages between Aug 18, 2011 and Oct 11, 2011. Diffs are folded; open one to read it.

replies: 17people: 4as markdown or json

David Aguilar· Aug 18, 2011, 07:23 UTC · lore

This series splits the git-mergetool--lib.sh file into separate tool scriptlets that define the commands for diff and merge.

David Aguilar (4):
  difftool--helper: Make style consistent with git
  mergetool--lib: Make style consistent with git
  mergetool--lib: Refactor tools into separate files
  mergetools/meld: Use '--output' when available
 Makefile                 |   11 ++
 git-difftool--helper.sh  |   18 ++-
 git-mergetool--lib.sh    |  394 ++++++++++------------------------------------
 mergetools/araxis        |   21 +++
 mergetools/bc3           |   20 +++
 mergetools/defaults      |   46 ++++++
 mergetools/diffuse       |   17 ++
 mergetools/ecmerge       |   16 ++
 mergetools/emerge        |   23 +++
 mergetools/kdiff3        |   24 +++
 mergetools/kompare       |    7 +
 mergetools/meld          |   40 +++++
 mergetools/opendiff      |   16 ++
 mergetools/p4merge       |   10 ++
 mergetools/tkdiff        |   12 ++
 mergetools/tortoisemerge |   17 ++
 mergetools/vim           |   44 +++++
 mergetools/xxdiff        |   25 +++
 18 files changed, 441 insertions(+), 320 deletions(-)
 create mode 100644 mergetools/araxis
 create mode 100644 mergetools/bc3
 create mode 100644 mergetools/defaults
 create mode 100644 mergetools/diffuse
 create mode 100644 mergetools/ecmerge
 create mode 100644 mergetools/emerge
 create mode 100644 mergetools/kdiff3
 create mode 100644 mergetools/kompare
 create mode 100644 mergetools/meld
 create mode 100644 mergetools/opendiff
 create mode 100644 mergetools/p4merge
 create mode 100644 mergetools/tkdiff
 create mode 100644 mergetools/tortoisemerge
 create mode 100644 mergetools/vim
 create mode 100644 mergetools/xxdiff

$ git describe --dirty v1.7.6-476-g572925c $ git version git version 1.7.6.476.g57292 $ ./t7610-mergetool.sh ok 1 - setup ok 2 - custom mergetool ok 3 - mergetool crlf ok 4 - mergetool in subdir ok 5 - mergetool on file in parent dir ok 6 - mergetool skips autoresolved ok 7 - mergetool merges all from subdir ok 8 - mergetool skips resolved paths when rerere is active ok 9 - deleted vs modified submodule ok 10 - file vs modified submodule ok 11 - submodule in subdirectory ok 12 - directory vs modified submodule # passed all 12 test(s) 1..12 $ ./t7800-difftool.sh ok 1 - setup ok 2 - custom commands ok 3 - difftool ignores bad --tool values ok 4 - difftool honors --gui ok 5 - difftool --gui works without configured diff.guitool ok 6 - GIT_DIFF_TOOL variable ok 7 - GIT_DIFF_TOOL overrides ok 8 - GIT_DIFFTOOL_NO_PROMPT variable ok 9 - GIT_DIFFTOOL_PROMPT variable ok 10 - difftool.prompt config variable is false ok 11 - difftool merge.prompt = false ok 12 - difftool.prompt can overridden with -y ok 13 - difftool.prompt can overridden with --prompt ok 14 - difftool last flag wins ok 15 - difftool + mergetool config variables ok 16 - difftool.<tool>.path ok 17 - difftool --extcmd=cat ok 18 - difftool --extcmd cat ok 19 - difftool -x cat ok 20 - difftool --extcmd echo arg1 ok 21 - difftool --extcmd cat arg1 ok 22 - difftool --extcmd cat arg2 # passed all 22 test(s) 1..22

-- 
1.7.6.476.g57292
David Aguilar· Aug 18, 2011, 07:23 UTC · re: David Aguilar · lore

[PATCH 1/4] difftool--helper: Make style consistent with git

Use the predominant conditional style where "then" appears alone on the line after the test expression.

Signed-off-by: David Aguilar <davvid@gmail.com>
---
 git-difftool--helper.sh |   18 ++++++++++++------
 1 files changed, 12 insertions(+), 6 deletions(-)
Show changes to git-difftool--helper.sh +12 −6
diff --git a/git-difftool--helper.sh b/git-difftool--helper.sh
index 0594bf7..8452890 100755
--- a/git-difftool--helper.sh
+++ b/git-difftool--helper.sh
@@ -13,7 +13,8 @@ TOOL_MODE=diff
 should_prompt () {
 	prompt_merge=$(git config --bool mergetool.prompt || echo true)
 	prompt=$(git config --bool difftool.prompt || echo $prompt_merge)
-	if test "$prompt" = true; then
+	if test "$prompt" = true
+	then
 		test -z "$GIT_DIFFTOOL_NO_PROMPT"
 	else
 		test -n "$GIT_DIFFTOOL_PROMPT"
@@ -37,9 +38,11 @@ launch_merge_tool () {
 
 	# $LOCAL and $REMOTE are temporary files so prompt
 	# the user with the real $MERGED name before launching $merge_tool.
-	if should_prompt; then
+	if should_prompt
+	then
 		printf "\nViewing: '$MERGED'\n"
-		if use_ext_cmd; then
+		if use_ext_cmd
+		then
 			printf "Hit return to launch '%s': " \
 				"$GIT_DIFFTOOL_EXTCMD"
 		else
@@ -48,7 +51,8 @@ launch_merge_tool () {
 		read ans
 	fi
 
-	if use_ext_cmd; then
+	if use_ext_cmd
+	then
 		export BASE
 		eval $GIT_DIFFTOOL_EXTCMD '"$LOCAL"' '"$REMOTE"'
 	else
@@ -56,8 +60,10 @@ launch_merge_tool () {
 	fi
 }
 
-if ! use_ext_cmd; then
-	if test -n "$GIT_DIFF_TOOL"; then
+if ! use_ext_cmd
+then
+	if test -n "$GIT_DIFF_TOOL"
+	then
 		merge_tool="$GIT_DIFF_TOOL"
 	else
 		merge_tool="$(get_merge_tool)" || exit

-- 
1.7.6.476.g57292
Sebastian Schuberth· Aug 18, 2011, 07:49 UTC · re: David Aguilar · lore

Re: [PATCH 1/4] difftool--helper: Make style consistent with git

On Thu, Aug 18, 2011 at 09:23, David Aguilar <davvid@gmail.com> wrote:
> Use the predominant conditional style where "then" appears
> alone on the line after the test expression.

I support your effort to unify the style, but for my personal taste it has gone the wrong way :-) Even if "then" on its own line was the predominant style in the merge scripts, I would have voted for putting all "then" in the same line as "if" as I find this easier to read and comprehend. But then again it's obvious that such a change would polarize ;-)

-- 
Sebastian Schuberth
David Aguilar· Aug 18, 2011, 09:08 UTC · re: Sebastian Schuberth · lore

Re: [PATCH 1/4] difftool--helper: Make style consistent with git

On Thu, Aug 18, 2011 at 09:49:33AM +0200, Sebastian Schuberth wrote:
Show 14 quoted lines
> On Thu, Aug 18, 2011 at 09:23, David Aguilar <davvid@gmail.com> wrote:
> 
> > Use the predominant conditional style where "then" appears
> > alone on the line after the test expression.
> 
> I support your effort to unify the style, but for my personal taste it
> has gone the wrong way :-) Even if "then" on its own line was the
> predominant style in the merge scripts, I would have voted for putting
> all "then" in the same line as "if" as I find this easier to read and
> comprehend. But then again it's obvious that such a change would
> polarize ;-)
> 
> -- 
> Sebastian Schuberth

I am fine reworking it either way. There's not a specific note about it in Documentation/CodingStyle.

If someone wants to make the final call I can patch the docs and rework these patches as needed.

Thanks,
-- 
					David
Junio C Hamano· Aug 18, 2011, 17:31 UTC · re: David Aguilar · lore

Re: [PATCH 1/4] difftool--helper: Make style consistent with git

David Aguilar <davvid@gmail.com> writes:
Show 6 quoted lines
> I am fine reworking it either way.
> There's not a specific note about it in
> Documentation/CodingStyle.
>
> If someone wants to make the final call I can patch the docs
> and rework these patches as needed.
Please stick to (and document if not):
	if test ...
        then
        	action
	elif condition that can become looong so more &&
        	condition needs to be split to next line &&
                even more condition
	then
		action
	else
		action
	fi
Thanks.
You can think of it as "war against semicolon" if you want.
David Aguilar· Aug 18, 2011, 07:23 UTC · re: David Aguilar · lore

[PATCH 2/4] mergetool--lib: Make style consistent with git

Use the predominant conditional style where "then" appears alone on the line after the test expression. Remove spaces after ">" output redirections. Remove unnecessary parentheses around the kdiff3 commands.

Signed-off-by: David Aguilar <davvid@gmail.com>
---
 git-mergetool--lib.sh |  153 ++++++++++++++++++++++++++++++++-----------------
 1 files changed, 100 insertions(+), 53 deletions(-)
Show changes to git-mergetool--lib.sh +100 −53
diff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh
index 91f90ac..03cfb19 100644
--- a/git-mergetool--lib.sh
+++ b/git-mergetool--lib.sh
@@ -32,10 +32,12 @@ translate_merge_tool_path () {
 }
 
 check_unchanged () {
-	if test "$MERGED" -nt "$BACKUP"; then
+	if test "$MERGED" -nt "$BACKUP"
+	then
 		status=0
 	else
-		while true; do
+		while true
+		do
 			echo "$MERGED seems unchanged."
 			printf "Was the merge successful? [y/n] "
 			read answer
@@ -53,17 +55,20 @@ valid_tool () {
 	kdiff3 | meld | opendiff | p4merge | tkdiff | vimdiff | vimdiff2 | xxdiff)
 		;; # happy
 	kompare)
-		if ! diff_mode; then
+		if ! diff_mode
+		then
 			return 1
 		fi
 		;;
 	tortoisemerge)
-		if ! merge_mode; then
+		if ! merge_mode
+		then
 			return 1
 		fi
 		;;
 	*)
-		if test -z "$(get_merge_tool_cmd "$1")"; then
+		if test -z "$(get_merge_tool_cmd "$1")"
+		then
 			return 1
 		fi
 		;;
@@ -72,12 +77,14 @@ valid_tool () {
 
 get_merge_tool_cmd () {
 	# Prints the custom command for a merge tool
-	if test -n "$1"; then
+	if test -n "$1"
+	then
 		merge_tool="$1"
 	else
 		merge_tool="$(get_merge_tool)"
 	fi
-	if diff_mode; then
+	if diff_mode
+	then
 		echo "$(git config difftool.$merge_tool.cmd ||
 		        git config mergetool.$merge_tool.cmd)"
 	else
@@ -97,9 +104,11 @@ run_merge_tool () {
 
 	case "$1" in
 	araxis)
-		if merge_mode; then
+		if merge_mode
+		then
 			touch "$BACKUP"
-			if $base_present; then
+			if $base_present
+			then
 				"$merge_tool_path" -wait -merge -3 -a1 \
 					"$BASE" "$LOCAL" "$REMOTE" "$MERGED" \
 					>/dev/null 2>&1
@@ -115,9 +124,11 @@ run_merge_tool () {
 		fi
 		;;
 	bc3)
-		if merge_mode; then
+		if merge_mode
+		then
 			touch "$BACKUP"
-			if $base_present; then
+			if $base_present
+			then
 				"$merge_tool_path" "$LOCAL" "$REMOTE" "$BASE" \
 					-mergeoutput="$MERGED"
 			else
@@ -130,9 +141,11 @@ run_merge_tool () {
 		fi
 		;;
 	diffuse)
-		if merge_mode; then
+		if merge_mode
+		then
 			touch "$BACKUP"
-			if $base_present; then
+			if $base_present
+			then
 				"$merge_tool_path" \
 					"$LOCAL" "$MERGED" "$REMOTE" \
 					"$BASE" | cat
@@ -146,9 +159,11 @@ run_merge_tool () {
 		fi
 		;;
 	ecmerge)
-		if merge_mode; then
+		if merge_mode
+		then
 			touch "$BACKUP"
-			if $base_present; then
+			if $base_present
+			then
 				"$merge_tool_path" "$BASE" "$LOCAL" "$REMOTE" \
 					--default --mode=merge3 --to="$MERGED"
 			else
@@ -162,8 +177,10 @@ run_merge_tool () {
 		fi
 		;;
 	emerge)
-		if merge_mode; then
-			if $base_present; then
+		if merge_mode
+		then
+			if $base_present
+			then
 				"$merge_tool_path" \
 					-f emerge-files-with-ancestor-command \
 					"$LOCAL" "$REMOTE" "$BASE" \
@@ -181,9 +198,11 @@ run_merge_tool () {
 		fi
 		;;
 	gvimdiff|vimdiff)
-		if merge_mode; then
+		if merge_mode
+		then
 			touch "$BACKUP"
-			if $base_present; then
+			if $base_present
+			then
 				"$merge_tool_path" -f -d -c "wincmd J" \
 					"$MERGED" "$LOCAL" "$BASE" "$REMOTE"
 			else
@@ -198,7 +217,8 @@ run_merge_tool () {
 		fi
 		;;
 	gvimdiff2|vimdiff2)
-		if merge_mode; then
+		if merge_mode
+		then
 			touch "$BACKUP"
 			"$merge_tool_path" -f -d -c "wincmd l" \
 				"$LOCAL" "$MERGED" "$REMOTE"
@@ -210,36 +230,39 @@ run_merge_tool () {
 		fi
 		;;
 	kdiff3)
-		if merge_mode; then
-			if $base_present; then
-				("$merge_tool_path" --auto \
+		if merge_mode
+		then
+			if $base_present
+			then
+				"$merge_tool_path" --auto \
 					--L1 "$MERGED (Base)" \
 					--L2 "$MERGED (Local)" \
 					--L3 "$MERGED (Remote)" \
 					-o "$MERGED" \
 					"$BASE" "$LOCAL" "$REMOTE" \
-				> /dev/null 2>&1)
+				>/dev/null 2>&1
 			else
-				("$merge_tool_path" --auto \
+				"$merge_tool_path" --auto \
 					--L1 "$MERGED (Local)" \
 					--L2 "$MERGED (Remote)" \
 					-o "$MERGED" \
 					"$LOCAL" "$REMOTE" \
-				> /dev/null 2>&1)
+				>/dev/null 2>&1
 			fi
 			status=$?
 		else
-			("$merge_tool_path" --auto \
+			"$merge_tool_path" --auto \
 				--L1 "$MERGED (A)" \
 				--L2 "$MERGED (B)" "$LOCAL" "$REMOTE" \
-			> /dev/null 2>&1)
+			>/dev/null 2>&1
 		fi
 		;;
 	kompare)
 		"$merge_tool_path" "$LOCAL" "$REMOTE"
 		;;
 	meld)
-		if merge_mode; then
+		if merge_mode
+		then
 			touch "$BACKUP"
 			"$merge_tool_path" "$LOCAL" "$MERGED" "$REMOTE"
 			check_unchanged
@@ -248,9 +271,11 @@ run_merge_tool () {
 		fi
 		;;
 	opendiff)
-		if merge_mode; then
+		if merge_mode
+		then
 			touch "$BACKUP"
-			if $base_present; then
+			if $base_present
+			then
 				"$merge_tool_path" "$LOCAL" "$REMOTE" \
 					-ancestor "$BASE" \
 					-merge "$MERGED" | cat
@@ -264,7 +289,8 @@ run_merge_tool () {
 		fi
 		;;
 	p4merge)
-		if merge_mode; then
+		if merge_mode
+		then
 			touch "$BACKUP"
 			$base_present || >"$BASE"
 			"$merge_tool_path" "$BASE" "$LOCAL" "$REMOTE" "$MERGED"
@@ -274,8 +300,10 @@ run_merge_tool () {
 		fi
 		;;
 	tkdiff)
-		if merge_mode; then
-			if $base_present; then
+		if merge_mode
+		then
+			if $base_present
+			then
 				"$merge_tool_path" -a "$BASE" \
 					-o "$MERGED" "$LOCAL" "$REMOTE"
 			else
@@ -288,7 +316,8 @@ run_merge_tool () {
 		fi
 		;;
 	tortoisemerge)
-		if $base_present; then
+		if $base_present
+		then
 			touch "$BACKUP"
 			"$merge_tool_path" \
 				-base:"$BASE" -mine:"$LOCAL" \
@@ -300,9 +329,11 @@ run_merge_tool () {
 		fi
 		;;
 	xxdiff)
-		if merge_mode; then
+		if merge_mode
+		then
 			touch "$BACKUP"
-			if $base_present; then
+			if $base_present
+			then
 				"$merge_tool_path" -X --show-merged-pane \
 					-R 'Accel.SaveAsMerged: "Ctrl-S"' \
 					-R 'Accel.Search: "Ctrl+F"' \
@@ -327,16 +358,20 @@ run_merge_tool () {
 		;;
 	*)
 		merge_tool_cmd="$(get_merge_tool_cmd "$1")"
-		if test -z "$merge_tool_cmd"; then
-			if merge_mode; then
+		if test -z "$merge_tool_cmd"
+		then
+			if merge_mode
+			then
 				status=1
 			fi
 			break
 		fi
-		if merge_mode; then
+		if merge_mode
+		then
 			trust_exit_code="$(git config --bool \
 				mergetool."$1".trustExitCode || echo false)"
-			if test "$trust_exit_code" = "false"; then
+			if test "$trust_exit_code" = "false"
+			then
 				touch "$BACKUP"
 				( eval $merge_tool_cmd )
 				check_unchanged
@@ -353,13 +388,16 @@ run_merge_tool () {
 }
 
 guess_merge_tool () {
-	if merge_mode; then
+	if merge_mode
+	then
 		tools="tortoisemerge"
 	else
 		tools="kompare"
 	fi
-	if test -n "$DISPLAY"; then
-		if test -n "$GNOME_DESKTOP_SESSION_ID" ; then
+	if test -n "$DISPLAY"
+	then
+		if test -n "$GNOME_DESKTOP_SESSION_ID"
+		then
 			tools="meld opendiff kdiff3 tkdiff xxdiff $tools"
 		else
 			tools="opendiff kdiff3 tkdiff xxdiff meld $tools"
@@ -380,7 +418,8 @@ guess_merge_tool () {
 	for i in $tools
 	do
 		merge_tool_path="$(translate_merge_tool_path "$i")"
-		if type "$merge_tool_path" > /dev/null 2>&1; then
+		if type "$merge_tool_path" >/dev/null 2>&1
+		then
 			echo "$i"
 			return 0
 		fi
@@ -393,12 +432,14 @@ guess_merge_tool () {
 get_configured_merge_tool () {
 	# Diff mode first tries diff.tool and falls back to merge.tool.
 	# Merge mode only checks merge.tool
-	if diff_mode; then
+	if diff_mode
+	then
 		merge_tool=$(git config diff.tool || git config merge.tool)
 	else
 		merge_tool=$(git config merge.tool)
 	fi
-	if test -n "$merge_tool" && ! valid_tool "$merge_tool"; then
+	if test -n "$merge_tool" && ! valid_tool "$merge_tool"
+	then
 		echo >&2 "git config option $TOOL_MODE.tool set to unknown tool: $merge_tool"
 		echo >&2 "Resetting to default..."
 		return 1
@@ -408,26 +449,31 @@ get_configured_merge_tool () {
 
 get_merge_tool_path () {
 	# A merge tool has been set, so verify that it's valid.
-	if test -n "$1"; then
+	if test -n "$1"
+	then
 		merge_tool="$1"
 	else
 		merge_tool="$(get_merge_tool)"
 	fi
-	if ! valid_tool "$merge_tool"; then
+	if ! valid_tool "$merge_tool"
+	then
 		echo >&2 "Unknown merge tool $merge_tool"
 		exit 1
 	fi
-	if diff_mode; then
+	if diff_mode
+	then
 		merge_tool_path=$(git config difftool."$merge_tool".path ||
 		                  git config mergetool."$merge_tool".path)
 	else
 		merge_tool_path=$(git config mergetool."$merge_tool".path)
 	fi
-	if test -z "$merge_tool_path"; then
+	if test -z "$merge_tool_path"
+	then
 		merge_tool_path="$(translate_merge_tool_path "$merge_tool")"
 	fi
 	if test -z "$(get_merge_tool_cmd "$merge_tool")" &&
-	! type "$merge_tool_path" > /dev/null 2>&1; then
+		! type "$merge_tool_path" >/dev/null 2>&1
+	then
 		echo >&2 "The $TOOL_MODE tool $merge_tool is not available as"\
 		         "'$merge_tool_path'"
 		exit 1
@@ -439,7 +485,8 @@ get_merge_tool () {
 	# Check if a merge tool has been configured
 	merge_tool=$(get_configured_merge_tool)
 	# Try to guess an appropriate merge tool if no tool has been set.
-	if test -z "$merge_tool"; then
+	if test -z "$merge_tool"
+	then
 		merge_tool="$(guess_merge_tool)" || exit
 	fi
 	echo "$merge_tool"

-- 
1.7.6.476.g57292
David Aguilar· Aug 18, 2011, 07:23 UTC · re: David Aguilar · lore

[PATCH 3/4] mergetool--lib: Refactor tools into separate files

Individual merge tools are now defined in a mergetools/$tool file which is sourced at runtime.

The individual files are installed into $(git --exec-path)/mergetools/. New tools can be added by creating a new file instead of editing the git-mergetool--lib.sh scriptlet.

http://thread.gmane.org/gmane.comp.version-control.git/134906/focus=135006
Signed-off-by: David Aguilar <davvid@gmail.com>
---
 Makefile                 |   11 ++
 git-mergetool--lib.sh    |  385 ++++++---------------------------------------
 mergetools/araxis        |   21 +++
 mergetools/bc3           |   20 +++
 mergetools/defaults      |   46 ++++++
 mergetools/diffuse       |   17 ++
 mergetools/ecmerge       |   16 ++
 mergetools/emerge        |   23 +++
 mergetools/kdiff3        |   24 +++
 mergetools/kompare       |    7 +
 mergetools/meld          |    9 +
 mergetools/opendiff      |   16 ++
 mergetools/p4merge       |   10 ++
 mergetools/tkdiff        |   12 ++
 mergetools/tortoisemerge |   17 ++
 mergetools/vim           |   44 ++++++
 mergetools/xxdiff        |   25 +++
 17 files changed, 370 insertions(+), 333 deletions(-)
 create mode 100644 mergetools/araxis
 create mode 100644 mergetools/bc3
 create mode 100644 mergetools/defaults
 create mode 100644 mergetools/diffuse
 create mode 100644 mergetools/ecmerge
 create mode 100644 mergetools/emerge
 create mode 100644 mergetools/kdiff3
 create mode 100644 mergetools/kompare
 create mode 100644 mergetools/meld
 create mode 100644 mergetools/opendiff
 create mode 100644 mergetools/p4merge
 create mode 100644 mergetools/tkdiff
 create mode 100644 mergetools/tortoisemerge
 create mode 100644 mergetools/vim
 create mode 100644 mergetools/xxdiff
Show changes to 17 files +370 −333

Makefile, git-mergetool--lib.sh, mergetools/araxis, mergetools/bc3, mergetools/defaults, mergetools/diffuse, mergetools/ecmerge, mergetools/emerge, mergetools/kdiff3, mergetools/kompare, mergetools/meld, mergetools/opendiff, mergetools/p4merge, mergetools/tkdiff, mergetools/tortoisemerge, mergetools/vim, mergetools/xxdiff

diff --git a/Makefile b/Makefile
index 8dd782f..6a68e3a 100644
--- a/Makefile
+++ b/Makefile
@@ -302,6 +302,7 @@ bindir = $(prefix)/$(bindir_relative)
 mandir = share/man
 infodir = share/info
 gitexecdir = libexec/git-core
+mergetoolsdir = $(gitexecdir)/mergetools
 sharedir = $(prefix)/share
 gitwebdir = $(sharedir)/gitweb
 template_dir = share/git-core/templates
@@ -2257,6 +2258,13 @@ endif
 gitexec_instdir_SQ = $(subst ','\'',$(gitexec_instdir))
 export gitexec_instdir
 
+ifneq ($(filter /%,$(firstword $(mergetoolsdir))),)
+mergetools_instdir = $(mergetoolsdir)
+else
+mergetools_instdir = $(prefix)/$(mergetoolsdir)
+endif
+mergetools_instdir_SQ = $(subst ','\'',$(mergetools_instdir))
+
 install_bindir_programs := $(patsubst %,%$X,$(BINDIR_PROGRAMS_NEED_X)) $(BINDIR_PROGRAMS_NO_X)
 
 install: all
@@ -2266,6 +2274,9 @@ install: all
 	$(INSTALL) -m 644 $(SCRIPT_LIB) '$(DESTDIR_SQ)$(gitexec_instdir_SQ)'
 	$(INSTALL) $(install_bindir_programs) '$(DESTDIR_SQ)$(bindir_SQ)'
 	$(MAKE) -C templates DESTDIR='$(DESTDIR_SQ)' install
+	$(INSTALL) -d -m 755 '$(DESTDIR_SQ)$(mergetools_instdir_SQ)'
+	(cd mergetools && $(TAR) cf - .) | \
+	(cd '$(DESTDIR_SQ)$(mergetools_instdir_SQ)' && umask 022 && $(TAR) xof -)
 ifndef NO_PERL
 	$(MAKE) -C perl prefix='$(prefix_SQ)' DESTDIR='$(DESTDIR_SQ)' install
 	$(MAKE) -C gitweb install
diff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh
index 03cfb19..2421700 100644
--- a/git-mergetool--lib.sh
+++ b/git-mergetool--lib.sh
@@ -9,26 +9,7 @@ merge_mode() {
 }
 
 translate_merge_tool_path () {
-	case "$1" in
-	araxis)
-		echo compare
-		;;
-	bc3)
-		echo bcompare
-		;;
-	emerge)
-		echo emacs
-		;;
-	gvimdiff|gvimdiff2)
-		echo gvim
-		;;
-	vimdiff|vimdiff2)
-		echo vim
-		;;
-	*)
-		echo "$1"
-		;;
-	esac
+	echo "$1"
 }
 
 check_unchanged () {
@@ -49,40 +30,55 @@ check_unchanged () {
 	fi
 }
 
+valid_tool_config () {
+	if test -n "$(get_merge_tool_cmd "$1")"
+	then
+		return 0
+	else
+		return 1
+	fi
+}
+
 valid_tool () {
+	setup_tool "$1" || valid_tool_config "$1"
+}
+
+setup_tool () {
 	case "$1" in
-	araxis | bc3 | diffuse | ecmerge | emerge | gvimdiff | gvimdiff2 | \
-	kdiff3 | meld | opendiff | p4merge | tkdiff | vimdiff | vimdiff2 | xxdiff)
-		;; # happy
-	kompare)
-		if ! diff_mode
-		then
-			return 1
-		fi
-		;;
-	tortoisemerge)
-		if ! merge_mode
-		then
-			return 1
-		fi
+	vim*|gvim*)
+		tool=vim
 		;;
 	*)
-		if test -z "$(get_merge_tool_cmd "$1")"
-		then
-			return 1
-		fi
+		tool="$1"
 		;;
 	esac
+	mergetools="$(git --exec-path)/mergetools"
+
+	# Load the default definitions
+	. "$mergetools/defaults"
+	if ! test -f "$mergetools/$tool"
+	then
+		return 1
+	fi
+
+	# Load the redefined functions
+	. "$mergetools/$tool"
+
+	if merge_mode && ! can_merge
+	then
+		echo "error: '$tool' can not be used to resolve merges" >&2
+		exit 1
+	elif diff_mode && ! can_diff
+	then
+		echo "error: '$tool' can only be used to resolve merges" >&2
+		exit 1
+	fi
+	return 0
 }
 
 get_merge_tool_cmd () {
 	# Prints the custom command for a merge tool
-	if test -n "$1"
-	then
-		merge_tool="$1"
-	else
-		merge_tool="$(get_merge_tool)"
-	fi
+	merge_tool="$1"
 	if diff_mode
 	then
 		echo "$(git config difftool.$merge_tool.cmd ||
@@ -92,6 +88,7 @@ get_merge_tool_cmd () {
 	fi
 }
 
+# Entry point for running tools
 run_merge_tool () {
 	# If GIT_PREFIX is empty then we cannot use it in tools
 	# that expect to be able to chdir() to its value.
@@ -102,288 +99,15 @@ run_merge_tool () {
 	base_present="$2"
 	status=0
 
-	case "$1" in
-	araxis)
-		if merge_mode
-		then
-			touch "$BACKUP"
-			if $base_present
-			then
-				"$merge_tool_path" -wait -merge -3 -a1 \
-					"$BASE" "$LOCAL" "$REMOTE" "$MERGED" \
-					>/dev/null 2>&1
-			else
-				"$merge_tool_path" -wait -2 \
-					"$LOCAL" "$REMOTE" "$MERGED" \
-					>/dev/null 2>&1
-			fi
-			check_unchanged
-		else
-			"$merge_tool_path" -wait -2 "$LOCAL" "$REMOTE" \
-				>/dev/null 2>&1
-		fi
-		;;
-	bc3)
-		if merge_mode
-		then
-			touch "$BACKUP"
-			if $base_present
-			then
-				"$merge_tool_path" "$LOCAL" "$REMOTE" "$BASE" \
-					-mergeoutput="$MERGED"
-			else
-				"$merge_tool_path" "$LOCAL" "$REMOTE" \
-					-mergeoutput="$MERGED"
-			fi
-			check_unchanged
-		else
-			"$merge_tool_path" "$LOCAL" "$REMOTE"
-		fi
-		;;
-	diffuse)
-		if merge_mode
-		then
-			touch "$BACKUP"
-			if $base_present
-			then
-				"$merge_tool_path" \
-					"$LOCAL" "$MERGED" "$REMOTE" \
-					"$BASE" | cat
-			else
-				"$merge_tool_path" \
-					"$LOCAL" "$MERGED" "$REMOTE" | cat
-			fi
-			check_unchanged
-		else
-			"$merge_tool_path" "$LOCAL" "$REMOTE" | cat
-		fi
-		;;
-	ecmerge)
-		if merge_mode
-		then
-			touch "$BACKUP"
-			if $base_present
-			then
-				"$merge_tool_path" "$BASE" "$LOCAL" "$REMOTE" \
-					--default --mode=merge3 --to="$MERGED"
-			else
-				"$merge_tool_path" "$LOCAL" "$REMOTE" \
-					--default --mode=merge2 --to="$MERGED"
-			fi
-			check_unchanged
-		else
-			"$merge_tool_path" --default --mode=diff2 \
-				"$LOCAL" "$REMOTE"
-		fi
-		;;
-	emerge)
-		if merge_mode
-		then
-			if $base_present
-			then
-				"$merge_tool_path" \
-					-f emerge-files-with-ancestor-command \
-					"$LOCAL" "$REMOTE" "$BASE" \
-					"$(basename "$MERGED")"
-			else
-				"$merge_tool_path" \
-					-f emerge-files-command \
-					"$LOCAL" "$REMOTE" \
-					"$(basename "$MERGED")"
-			fi
-			status=$?
-		else
-			"$merge_tool_path" -f emerge-files-command \
-				"$LOCAL" "$REMOTE"
-		fi
-		;;
-	gvimdiff|vimdiff)
-		if merge_mode
-		then
-			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
-		else
-			"$merge_tool_path" -R -f -d -c "wincmd l" \
-				-c 'cd $GIT_PREFIX' \
-				"$LOCAL" "$REMOTE"
-		fi
-		;;
-	gvimdiff2|vimdiff2)
-		if merge_mode
-		then
-			touch "$BACKUP"
-			"$merge_tool_path" -f -d -c "wincmd l" \
-				"$LOCAL" "$MERGED" "$REMOTE"
-			check_unchanged
-		else
-			"$merge_tool_path" -R -f -d -c "wincmd l" \
-				-c 'cd $GIT_PREFIX' \
-				"$LOCAL" "$REMOTE"
-		fi
-		;;
-	kdiff3)
-		if merge_mode
-		then
-			if $base_present
-			then
-				"$merge_tool_path" --auto \
-					--L1 "$MERGED (Base)" \
-					--L2 "$MERGED (Local)" \
-					--L3 "$MERGED (Remote)" \
-					-o "$MERGED" \
-					"$BASE" "$LOCAL" "$REMOTE" \
-				>/dev/null 2>&1
-			else
-				"$merge_tool_path" --auto \
-					--L1 "$MERGED (Local)" \
-					--L2 "$MERGED (Remote)" \
-					-o "$MERGED" \
-					"$LOCAL" "$REMOTE" \
-				>/dev/null 2>&1
-			fi
-			status=$?
-		else
-			"$merge_tool_path" --auto \
-				--L1 "$MERGED (A)" \
-				--L2 "$MERGED (B)" "$LOCAL" "$REMOTE" \
-			>/dev/null 2>&1
-		fi
-		;;
-	kompare)
-		"$merge_tool_path" "$LOCAL" "$REMOTE"
-		;;
-	meld)
-		if merge_mode
-		then
-			touch "$BACKUP"
-			"$merge_tool_path" "$LOCAL" "$MERGED" "$REMOTE"
-			check_unchanged
-		else
-			"$merge_tool_path" "$LOCAL" "$REMOTE"
-		fi
-		;;
-	opendiff)
-		if merge_mode
-		then
-			touch "$BACKUP"
-			if $base_present
-			then
-				"$merge_tool_path" "$LOCAL" "$REMOTE" \
-					-ancestor "$BASE" \
-					-merge "$MERGED" | cat
-			else
-				"$merge_tool_path" "$LOCAL" "$REMOTE" \
-					-merge "$MERGED" | cat
-			fi
-			check_unchanged
-		else
-			"$merge_tool_path" "$LOCAL" "$REMOTE" | cat
-		fi
-		;;
-	p4merge)
-		if merge_mode
-		then
-			touch "$BACKUP"
-			$base_present || >"$BASE"
-			"$merge_tool_path" "$BASE" "$LOCAL" "$REMOTE" "$MERGED"
-			check_unchanged
-		else
-			"$merge_tool_path" "$LOCAL" "$REMOTE"
-		fi
-		;;
-	tkdiff)
-		if merge_mode
-		then
-			if $base_present
-			then
-				"$merge_tool_path" -a "$BASE" \
-					-o "$MERGED" "$LOCAL" "$REMOTE"
-			else
-				"$merge_tool_path" \
-					-o "$MERGED" "$LOCAL" "$REMOTE"
-			fi
-			status=$?
-		else
-			"$merge_tool_path" "$LOCAL" "$REMOTE"
-		fi
-		;;
-	tortoisemerge)
-		if $base_present
-		then
-			touch "$BACKUP"
-			"$merge_tool_path" \
-				-base:"$BASE" -mine:"$LOCAL" \
-				-theirs:"$REMOTE" -merged:"$MERGED"
-			check_unchanged
-		else
-			echo "TortoiseMerge cannot be used without a base" 1>&2
-			status=1
-		fi
-		;;
-	xxdiff)
-		if merge_mode
-		then
-			touch "$BACKUP"
-			if $base_present
-			then
-				"$merge_tool_path" -X --show-merged-pane \
-					-R 'Accel.SaveAsMerged: "Ctrl-S"' \
-					-R 'Accel.Search: "Ctrl+F"' \
-					-R 'Accel.SearchForward: "Ctrl-G"' \
-					--merged-file "$MERGED" \
-					"$LOCAL" "$BASE" "$REMOTE"
-			else
-				"$merge_tool_path" -X $extra \
-					-R 'Accel.SaveAsMerged: "Ctrl-S"' \
-					-R 'Accel.Search: "Ctrl+F"' \
-					-R 'Accel.SearchForward: "Ctrl-G"' \
-					--merged-file "$MERGED" \
-					"$LOCAL" "$REMOTE"
-			fi
-			check_unchanged
-		else
-			"$merge_tool_path" \
-				-R 'Accel.Search: "Ctrl+F"' \
-				-R 'Accel.SearchForward: "Ctrl-G"' \
-				"$LOCAL" "$REMOTE"
-		fi
-		;;
-	*)
-		merge_tool_cmd="$(get_merge_tool_cmd "$1")"
-		if test -z "$merge_tool_cmd"
-		then
-			if merge_mode
-			then
-				status=1
-			fi
-			break
-		fi
-		if merge_mode
-		then
-			trust_exit_code="$(git config --bool \
-				mergetool."$1".trustExitCode || echo false)"
-			if test "$trust_exit_code" = "false"
-			then
-				touch "$BACKUP"
-				( eval $merge_tool_cmd )
-				check_unchanged
-			else
-				( eval $merge_tool_cmd )
-				status=$?
-			fi
-		else
-			( eval $merge_tool_cmd )
-		fi
-		;;
-	esac
+	# Bring tool-specific functions into scope
+	setup_tool "$1"
+
+	if merge_mode
+	then
+		merge_cmd "$1"
+	else
+		diff_cmd "$1"
+	fi
 	return $status
 }
 
@@ -449,12 +173,7 @@ get_configured_merge_tool () {
 
 get_merge_tool_path () {
 	# A merge tool has been set, so verify that it's valid.
-	if test -n "$1"
-	then
-		merge_tool="$1"
-	else
-		merge_tool="$(get_merge_tool)"
-	fi
+	merge_tool="$1"
 	if ! valid_tool "$merge_tool"
 	then
 		echo >&2 "Unknown merge tool $merge_tool"
@@ -483,7 +202,7 @@ get_merge_tool_path () {
 
 get_merge_tool () {
 	# Check if a merge tool has been configured
-	merge_tool=$(get_configured_merge_tool)
+	merge_tool="$(get_configured_merge_tool)"
 	# Try to guess an appropriate merge tool if no tool has been set.
 	if test -z "$merge_tool"
 	then
diff --git a/mergetools/araxis b/mergetools/araxis
new file mode 100644
index 0000000..69a9c8c
--- /dev/null
+++ b/mergetools/araxis
@@ -0,0 +1,21 @@
+diff_cmd () {
+	"$merge_tool_path" -wait -2 "$LOCAL" "$REMOTE" >/dev/null 2>&1
+}
+
+merge_cmd () {
+	touch "$BACKUP"
+	if $base_present
+	then
+		"$merge_tool_path" -wait -merge -3 -a1 \
+			"$BASE" "$LOCAL" "$REMOTE" "$MERGED" >/dev/null 2>&1
+	else
+		"$merge_tool_path" -wait -2 \
+			"$LOCAL" "$REMOTE" "$MERGED" >/dev/null 2>&1
+	fi
+	check_unchanged
+}
+
+translate_merge_tool_path() {
+	echo compare
+}
+
diff --git a/mergetools/bc3 b/mergetools/bc3
new file mode 100644
index 0000000..27b3dd4
--- /dev/null
+++ b/mergetools/bc3
@@ -0,0 +1,20 @@
+diff_cmd () {
+	"$merge_tool_path" "$LOCAL" "$REMOTE"
+}
+
+merge_cmd () {
+	touch "$BACKUP"
+	if $base_present
+	then
+		"$merge_tool_path" "$LOCAL" "$REMOTE" "$BASE" \
+			-mergeoutput="$MERGED"
+	else
+		"$merge_tool_path" "$LOCAL" "$REMOTE" \
+			-mergeoutput="$MERGED"
+	fi
+	check_unchanged
+}
+
+translate_merge_tool_path() {
+	echo bcompare
+}
diff --git a/mergetools/defaults b/mergetools/defaults
new file mode 100644
index 0000000..1d8f2a3
--- /dev/null
+++ b/mergetools/defaults
@@ -0,0 +1,46 @@
+# Redefined by builtin tools
+can_merge () {
+	return 0
+}
+
+can_diff () {
+	return 0
+}
+
+diff_cmd () {
+	merge_tool_cmd="$(get_merge_tool_cmd "$1")"
+	if test -z "$merge_tool_cmd"
+	then
+		status=1
+		break
+	fi
+	( eval $merge_tool_cmd )
+	status=$?
+	return $status
+}
+
+merge_cmd () {
+	merge_tool_cmd="$(get_merge_tool_cmd "$1")"
+	if test -z "$merge_tool_cmd"
+	then
+		status=1
+		break
+	fi
+	trust_exit_code="$(git config --bool \
+		mergetool."$1".trustExitCode || echo false)"
+	if test "$trust_exit_code" = "false"
+	then
+		touch "$BACKUP"
+		( eval $merge_tool_cmd )
+		status=$?
+		check_unchanged
+	else
+		( eval $merge_tool_cmd )
+		status=$?
+	fi
+	return $status
+}
+
+translate_merge_tool_path () {
+	echo "$1"
+}
diff --git a/mergetools/diffuse b/mergetools/diffuse
new file mode 100644
index 0000000..02e0843
--- /dev/null
+++ b/mergetools/diffuse
@@ -0,0 +1,17 @@
+diff_cmd () {
+	"$merge_tool_path" "$LOCAL" "$REMOTE" | cat
+}
+
+merge_cmd () {
+	touch "$BACKUP"
+	if $base_present
+	then
+		"$merge_tool_path" \
+			"$LOCAL" "$MERGED" "$REMOTE" \
+			"$BASE" | cat
+	else
+		"$merge_tool_path" \
+			"$LOCAL" "$MERGED" "$REMOTE" | cat
+	fi
+	check_unchanged
+}
diff --git a/mergetools/ecmerge b/mergetools/ecmerge
new file mode 100644
index 0000000..13c2e43
--- /dev/null
+++ b/mergetools/ecmerge
@@ -0,0 +1,16 @@
+diff_cmd () {
+	"$merge_tool_path" --default --mode=diff2 "$LOCAL" "$REMOTE"
+}
+
+merge_cmd () {
+	touch "$BACKUP"
+	if $base_present
+	then
+		"$merge_tool_path" "$BASE" "$LOCAL" "$REMOTE" \
+			--default --mode=merge3 --to="$MERGED"
+	else
+		"$merge_tool_path" "$LOCAL" "$REMOTE" \
+			--default --mode=merge2 --to="$MERGED"
+	fi
+	check_unchanged
+}
diff --git a/mergetools/emerge b/mergetools/emerge
new file mode 100644
index 0000000..f96d9e5
--- /dev/null
+++ b/mergetools/emerge
@@ -0,0 +1,23 @@
+diff_cmd () {
+	"$merge_tool_path" -f emerge-files-command "$LOCAL" "$REMOTE"
+}
+
+merge_cmd () {
+	if $base_present
+	then
+		"$merge_tool_path" \
+			-f emerge-files-with-ancestor-command \
+			"$LOCAL" "$REMOTE" "$BASE" \
+			"$(basename "$MERGED")"
+	else
+		"$merge_tool_path" \
+			-f emerge-files-command \
+			"$LOCAL" "$REMOTE" \
+			"$(basename "$MERGED")"
+	fi
+	status=$?
+}
+
+translate_merge_tool_path() {
+	echo emacs
+}
diff --git a/mergetools/kdiff3 b/mergetools/kdiff3
new file mode 100644
index 0000000..28fead4
--- /dev/null
+++ b/mergetools/kdiff3
@@ -0,0 +1,24 @@
+diff_cmd () {
+	"$merge_tool_path" --auto \
+		--L1 "$MERGED (A)" --L2 "$MERGED (B)" \
+		"$LOCAL" "$REMOTE" >/dev/null 2>&1
+}
+
+merge_cmd () {
+	if $base_present
+	then
+		"$merge_tool_path" --auto \
+			--L1 "$MERGED (Base)" \
+			--L2 "$MERGED (Local)" \
+			--L3 "$MERGED (Remote)" \
+			-o "$MERGED" "$BASE" "$LOCAL" "$REMOTE" \
+		>/dev/null 2>&1
+	else
+		"$merge_tool_path" --auto \
+			--L1 "$MERGED (Local)" \
+			--L2 "$MERGED (Remote)" \
+			-o "$MERGED" "$LOCAL" "$REMOTE" \
+		>/dev/null 2>&1
+	fi
+	status=$?
+}
diff --git a/mergetools/kompare b/mergetools/kompare
new file mode 100644
index 0000000..433686c
--- /dev/null
+++ b/mergetools/kompare
@@ -0,0 +1,7 @@
+can_merge () {
+	return 1
+}
+
+diff_cmd () {
+	"$merge_tool_path" "$LOCAL" "$REMOTE"
+}
diff --git a/mergetools/meld b/mergetools/meld
new file mode 100644
index 0000000..73d70ae
--- /dev/null
+++ b/mergetools/meld
@@ -0,0 +1,9 @@
+diff_cmd () {
+	"$merge_tool_path" "$LOCAL" "$REMOTE"
+}
+
+merge_cmd () {
+	touch "$BACKUP"
+	"$merge_tool_path" "$LOCAL" "$MERGED" "$REMOTE"
+	check_unchanged
+}
diff --git a/mergetools/opendiff b/mergetools/opendiff
new file mode 100644
index 0000000..0942b2a
--- /dev/null
+++ b/mergetools/opendiff
@@ -0,0 +1,16 @@
+diff_cmd () {
+	"$merge_tool_path" "$LOCAL" "$REMOTE" | cat
+}
+
+merge_cmd () {
+	touch "$BACKUP"
+	if $base_present
+	then
+		"$merge_tool_path" "$LOCAL" "$REMOTE" \
+			-ancestor "$BASE" -merge "$MERGED" | cat
+	else
+		"$merge_tool_path" "$LOCAL" "$REMOTE" \
+			-merge "$MERGED" | cat
+	fi
+	check_unchanged
+}
diff --git a/mergetools/p4merge b/mergetools/p4merge
new file mode 100644
index 0000000..1a45c1b
--- /dev/null
+++ b/mergetools/p4merge
@@ -0,0 +1,10 @@
+diff_cmd () {
+	"$merge_tool_path" "$LOCAL" "$REMOTE"
+}
+
+merge_cmd () {
+	touch "$BACKUP"
+	$base_present || >"$BASE"
+	"$merge_tool_path" "$BASE" "$LOCAL" "$REMOTE" "$MERGED"
+	check_unchanged
+}
diff --git a/mergetools/tkdiff b/mergetools/tkdiff
new file mode 100644
index 0000000..618c438
--- /dev/null
+++ b/mergetools/tkdiff
@@ -0,0 +1,12 @@
+diff_cmd () {
+	"$merge_tool_path" "$LOCAL" "$REMOTE"
+}
+
+merge_cmd () {
+	if $base_present
+	then
+		"$merge_tool_path" -a "$BASE" -o "$MERGED" "$LOCAL" "$REMOTE"
+	else
+		"$merge_tool_path" -o "$MERGED" "$LOCAL" "$REMOTE"
+	fi
+}
diff --git a/mergetools/tortoisemerge b/mergetools/tortoisemerge
new file mode 100644
index 0000000..ed7db49
--- /dev/null
+++ b/mergetools/tortoisemerge
@@ -0,0 +1,17 @@
+can_diff () {
+	return 1
+}
+
+merge_cmd () {
+	if $base_present
+	then
+		touch "$BACKUP"
+		"$merge_tool_path" \
+			-base:"$BASE" -mine:"$LOCAL" \
+			-theirs:"$REMOTE" -merged:"$MERGED"
+		check_unchanged
+	else
+		echo "TortoiseMerge cannot be used without a base" 1>&2
+		return 1
+	fi
+}
diff --git a/mergetools/vim b/mergetools/vim
new file mode 100644
index 0000000..619594a
--- /dev/null
+++ b/mergetools/vim
@@ -0,0 +1,44 @@
+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_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)
+		"$merge_tool_path" -f -d -c 'wincmd l' \
+			"$LOCAL" "$MERGED" "$REMOTE"
+		;;
+	esac
+	check_unchanged
+}
+
+translate_merge_tool_path() {
+	case "$1" in
+	gvimdiff|gvimdiff2)
+		echo gvim
+		;;
+	vimdiff|vimdiff2)
+		echo vim
+		;;
+	esac
+}
diff --git a/mergetools/xxdiff b/mergetools/xxdiff
new file mode 100644
index 0000000..05b4433
--- /dev/null
+++ b/mergetools/xxdiff
@@ -0,0 +1,25 @@
+diff_cmd () {
+	"$merge_tool_path" \
+		-R 'Accel.Search: "Ctrl+F"' \
+		-R 'Accel.SearchForward: "Ctrl-G"' \
+		"$LOCAL" "$REMOTE"
+}
+
+merge_cmd () {
+	touch "$BACKUP"
+	if $base_present
+	then
+		"$merge_tool_path" -X --show-merged-pane \
+			-R 'Accel.SaveAsMerged: "Ctrl-S"' \
+			-R 'Accel.Search: "Ctrl+F"' \
+			-R 'Accel.SearchForward: "Ctrl-G"' \
+			--merged-file "$MERGED" "$LOCAL" "$BASE" "$REMOTE"
+	else
+		"$merge_tool_path" -X $extra \
+			-R 'Accel.SaveAsMerged: "Ctrl-S"' \
+			-R 'Accel.Search: "Ctrl+F"' \
+			-R 'Accel.SearchForward: "Ctrl-G"' \
+			--merged-file "$MERGED" "$LOCAL" "$REMOTE"
+	fi
+	check_unchanged
+}

-- 
1.7.6.476.g57292
Jonathan Nieder· Oct 9, 2011, 09:17 UTC · re: David Aguilar · lore

[PATCH da/difftool-mergtool-refactor] Makefile: fix permissions of mergetools/ checked out with permissive umask

Ever since mergetool--lib was split into multiple files in v1.7.7-rc0~3^2~1 (2011-08-18), the Makefile takes care to reset umask and use tar --no-owner when installing merge tool definitions to $(gitexecdir)/mergetools/. Unfortunately it does not take into account the possibility that the permission bits of the files being copied might already be wrong.

Rather than fixing the "tar" incantation and making it even more complicated, let's just use the "install" utility. This only means losing the ability to install executables and subdirectories of mergetools/, which wasn't used.

Noticed by installing from a copy of git checked out with umask 002. Compare v1.6.0.3~81^2 (Fix permission bits on sources checked out with an overtight umask, 2008-08-21).

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
David Aguilar wrote:
> +++ b/Makefile
[...]
Show 7 quoted lines
> @@ -2266,6 +2274,9 @@ install: all
>  	$(INSTALL) -m 644 $(SCRIPT_LIB) '$(DESTDIR_SQ)$(gitexec_instdir_SQ)'
>  	$(INSTALL) $(install_bindir_programs) '$(DESTDIR_SQ)$(bindir_SQ)'
>  	$(MAKE) -C templates DESTDIR='$(DESTDIR_SQ)' install
> +	$(INSTALL) -d -m 755 '$(DESTDIR_SQ)$(mergetools_instdir_SQ)'
> +	(cd mergetools && $(TAR) cf - .) | \
> +	(cd '$(DESTDIR_SQ)$(mergetools_instdir_SQ)' && umask 022 && $(TAR) xof -)

Last month I tried this out and found that, strangely, my files under /usr/lib/git-core/mergetools/ had the g+w bit set. Leading me to wonder: does the "umask" here have any effect at all?

Since debian/rules install is run as root, the default is for tar to act as thought --preserve-permissions were passed, so the umask when running "tar" is not relevant. Luckily I think "tar" is overkill here, anyway.

Thoughts?  Sorry to have taken so long to send this out.
 Makefile |    3 +--
 1 files changed, 1 insertions(+), 2 deletions(-)
Show changes to Makefile +1 −2
diff --git a/Makefile b/Makefile
index 1e91b19c..e27755e7 100644
--- a/Makefile
+++ b/Makefile
@@ -2275,8 +2275,7 @@ install: all
 	$(INSTALL) $(install_bindir_programs) '$(DESTDIR_SQ)$(bindir_SQ)'
 	$(MAKE) -C templates DESTDIR='$(DESTDIR_SQ)' install
 	$(INSTALL) -d -m 755 '$(DESTDIR_SQ)$(mergetools_instdir_SQ)'
-	(cd mergetools && $(TAR) cf - .) | \
-	(cd '$(DESTDIR_SQ)$(mergetools_instdir_SQ)' && umask 022 && $(TAR) xof -)
+	$(INSTALL) -m 644 mergetools/* '$(DESTDIR_SQ)$(mergetools_instdir_SQ)'
 ifndef NO_PERL
 	$(MAKE) -C perl prefix='$(prefix_SQ)' DESTDIR='$(DESTDIR_SQ)' install
 	$(MAKE) -C gitweb install

-- 
1.7.7.rc1
Jonathan Nieder· Oct 9, 2011, 11:43 UTC · re: Jonathan Nieder · lore

Re: [PATCH da/difftool-mergtool-refactor] Makefile: fix permissions of mergetools/ checked out with permissive umask

Jonathan Nieder wrote:
> ---
[...]
> Since debian/rules install is run as root, the default is for tar to
> act as thought --preserve-permissions were passed
I should have said: "when 'make install' is run as root, ...".

Typically people building git for private use would run "make install" as root when installing to /usr/local, but as an unprivileged user when installing to $HOME. The RPM packaging runs "make install" without special privileges and the Debian packaging runs it as (fake) root, iirc.

Sorry for the lack of clarity.
David Aguilar· Oct 9, 2011, 18:26 UTC · re: Jonathan Nieder · lore

Re: [PATCH da/difftool-mergtool-refactor] Makefile: fix permissions of mergetools/ checked out with permissive umask

On Oct 9, 2011, at 4:43 AM, Jonathan Nieder <jrnieder@gmail.com> wrote:
Show 16 quoted lines
> Jonathan Nieder wrote:
> 
>> ---
> [...]
>> Since debian/rules install is run as root, the default is for tar to
>> act as thought --preserve-permissions were passed
> 
> I should have said: "when 'make install' is run as root, ...".
> 
> Typically people building git for private use would run "make install"
> as root when installing to /usr/local, but as an unprivileged user
> when installing to $HOME.  The RPM packaging runs "make install"
> without special privileges and the Debian packaging runs it as (fake)
> root, iirc.
> 
> Sorry for the lack of clarity.
thanks. I agree that the tar is overkill. I think I copied that snippet from templates/makefile. does that have the same bug?
-- 
    David (mobile)
Jonathan Nieder· Oct 9, 2011, 21:47 UTC · re: David Aguilar · lore

Re: [PATCH da/difftool-mergtool-refactor] Makefile: fix permissions of mergetools/ checked out with permissive umask

David Aguilar wrote:
> thanks. I agree that the tar is overkill. I think I copied that
> snippet from templates/makefile. does that have the same bug?

It did have a similar bug before, but it was fixed (in a different way) by v1.6.0.3~81^2 (Fix permission bits on sources checked out with an overtight umask, 2008-08-21). The main difference from the mergetools/ case is that the blt/ directory is populated at build time.

Junio C Hamano· Oct 11, 2011, 00:00 UTC · re: David Aguilar · lore

Re: [PATCH da/difftool-mergtool-refactor] Makefile: fix permissions of mergetools/ checked out with permissive umask

David Aguilar <davvid@gmail.com> writes:
> thanks. I agree that the tar is overkill. I think I copied that snippet
> from templates/makefile. does that have the same bug?

Likely, as I recall that I wrote the installation of templates in an expecially sloppy way, thinking somebody would fix it in short order anyway. Instead the sloppiness seems to have been copied and pasted to make things worse...

Thanks for fixing the mess---all's well that ends well.
David Aguilar· Aug 18, 2011, 07:23 UTC · re: David Aguilar · lore

[PATCH 4/4] mergetools/meld: Use '--output' when available

meld 1.5.0 and newer allow the output file to be specified when merging multiple files. Check the meld version and use the '--output' option when available.

Signed-off-by: David Aguilar <davvid@gmail.com>
---
 mergetools/meld |   33 ++++++++++++++++++++++++++++++++-
 1 files changed, 32 insertions(+), 1 deletions(-)
Show changes to mergetools/meld +32 −1
diff --git a/mergetools/meld b/mergetools/meld
index 73d70ae..1923797 100644
--- a/mergetools/meld
+++ b/mergetools/meld
@@ -4,6 +4,37 @@ diff_cmd () {
 
 merge_cmd () {
 	touch "$BACKUP"
-	"$merge_tool_path" "$LOCAL" "$MERGED" "$REMOTE"
+	if test "$meld_has_output_option" = true
+	then
+		"$merge_tool_path" --output "$MERGED" \
+			"$BASE" "$LOCAL" "$REMOTE"
+	else
+		"$merge_tool_path" "$LOCAL" "$MERGED" "$REMOTE"
+	fi
 	check_unchanged
 }
+
+check_meld_version () {
+	if test -n "$meld_version_checked"
+	then
+		return
+	fi
+	# Whether 'meld --output <file>' is supported
+	meld_has_output_option=false
+
+	# Filter meld --version to contain numbers and dots only
+	meld="$(meld --version 2>/dev/null | sed -e 's/[^0-9.]\+//g')"
+	meld="${meld:-0.0.0}"
+
+	meld_major="$(expr "$meld" : '\([0-9]\{1,\}\)' || echo 0)"
+	meld_minor="$(expr "$meld" : '[0-9]\{1,\}\.\([0-9]\{1,\}\)' || echo 0)"
+
+	# 'meld --output <file>' was introduced in meld 1.5.0
+	if test "$meld_major" -gt 1 ||
+		(test "$meld_major" = 1 && test "$meld_minor" -ge 5)
+	then
+		meld_has_output_option=true
+	fi
+	meld_version_checked=true
+}
+check_meld_version

-- 
1.7.6.476.g57292
Jonathan Nieder· Aug 18, 2011, 08:13 UTC · re: David Aguilar · lore

Re: [PATCH 4/4] mergetools/meld: Use '--output' when available

David Aguilar wrote:
> use the '--output' option when available.
Yay. :)
Show 11 quoted lines
> --- a/mergetools/meld
> +++ b/mergetools/meld
> @@ -4,6 +4,37 @@ diff_cmd () {
>  
>  merge_cmd () {
>  	touch "$BACKUP"
> -	"$merge_tool_path" "$LOCAL" "$MERGED" "$REMOTE"
> +	if test "$meld_has_output_option" = true
> +	then
> +		"$merge_tool_path" --output "$MERGED" \
> +			"$BASE" "$LOCAL" "$REMOTE"
Shouldn't this be "$LOCAL" "$BASE" "$REMOTE"?
> +	else
> +		"$merge_tool_path" "$LOCAL" "$MERGED" "$REMOTE"
> +	fi
>  	check_unchanged

I wonder if the version test could be made a little simpler (perhaps to cope better if future versions use a different numbering system):

	if "$merge_tool_path" --output /dev/null --help >/dev/null 2>&1
	then
		"$merge_tool_path" --output ...
	else
		"$merge_tool_path" "$LOCAL" ...
	fi

Forgive my ignorance: is this function likely to be called in a loop? If so, it makes sense to precompute or cache the result of detection, like you already do.

	check_meld_for_output_option () {
		if ...
		then
			meld_has_output_option=true
		else
			meld_has_output_option=false
		fi
	}
	merge_cmd () {
		if test -z "${meld_has_output_option:+set}"
		then
			check_meld_for_output_option
		fi
		if test "$meld_has_output_option" = true
		then
			...
[...]
> +	# Filter meld --version to contain numbers and dots only
> +	meld="$(meld --version 2>/dev/null | sed -e 's/[^0-9.]\+//g')"

\+ is not a BRE. If parsing version numbers seems like the right thing to do, maybe "tr -cd 0-9."?

> +	meld="${meld:-0.0.0}"
> +
> +	meld_major="$(expr "$meld" : '\([0-9]\{1,\}\)' || echo 0)"
> +	meld_minor="$(expr "$meld" : '[0-9]\{1,\}\.\([0-9]\{1,\}\)' || echo 0)"

I think git avoids \{m,n\} ranges where possible (for portability). This could be:

	meld_major=${meld%%.*}
	meld_nonmajor=${meld#${meld_major}.}
	meld_minor=${meld_nonmajor%%.*}
or:
	case $meld in
	[2-9].* | [1-9][0-9]* | 1.[5-9]* | 1.[1-9][0-9]*)	# >= 1.5.0
		meld_has_output_option=true ;;
	*)
		meld_has_output_option=false ;;
	esac

It's nice how self-contained this can be now that it's in its own file. Thanks.

David Aguilar· Aug 18, 2011, 09:24 UTC · re: Jonathan Nieder · lore

Re: [PATCH 4/4] mergetools/meld: Use '--output' when available

On Thu, Aug 18, 2011 at 03:13:10AM -0500, Jonathan Nieder wrote:
Show 19 quoted lines
> David Aguilar wrote:
> 
> > use the '--output' option when available.
> 
> Yay. :)
> 
> > --- a/mergetools/meld
> > +++ b/mergetools/meld
> > @@ -4,6 +4,37 @@ diff_cmd () {
> >  
> >  merge_cmd () {
> >  	touch "$BACKUP"
> > -	"$merge_tool_path" "$LOCAL" "$MERGED" "$REMOTE"
> > +	if test "$meld_has_output_option" = true
> > +	then
> > +		"$merge_tool_path" --output "$MERGED" \
> > +			"$BASE" "$LOCAL" "$REMOTE"
> 
> Shouldn't this be "$LOCAL" "$BASE" "$REMOTE"?
Yup, thanks.
Show 19 quoted lines
> 
> > +	else
> > +		"$merge_tool_path" "$LOCAL" "$MERGED" "$REMOTE"
> > +	fi
> >  	check_unchanged
> 
> I wonder if the version test could be made a little simpler (perhaps
> to cope better if future versions use a different numbering system):
> 
> 	if "$merge_tool_path" --output /dev/null --help >/dev/null 2>&1
> 	then
> 		"$merge_tool_path" --output ...
> 	else
> 		"$merge_tool_path" "$LOCAL" ...
> 	fi
> 
> Forgive my ignorance: is this function likely to be called in a loop?
> If so, it makes sense to precompute or cache the result of detection,
> like you already do.
Yes, it is called in a loop...
Show 50 quoted lines
> 	check_meld_for_output_option () {
> 		if ...
> 		then
> 			meld_has_output_option=true
> 		else
> 			meld_has_output_option=false
> 		fi
> 	}
> 
> 	merge_cmd () {
> 		if test -z "${meld_has_output_option:+set}"
> 		then
> 			check_meld_for_output_option
> 		fi
> 
> 		if test "$meld_has_output_option" = true
> 		then
> 			...
> 
> 
> [...]
> > +	# Filter meld --version to contain numbers and dots only
> > +	meld="$(meld --version 2>/dev/null | sed -e 's/[^0-9.]\+//g')"
> 
> \+ is not a BRE.  If parsing version numbers seems like the right
> thing to do, maybe "tr -cd 0-9."?
> 
> > +	meld="${meld:-0.0.0}"
> > +
> > +	meld_major="$(expr "$meld" : '\([0-9]\{1,\}\)' || echo 0)"
> > +	meld_minor="$(expr "$meld" : '[0-9]\{1,\}\.\([0-9]\{1,\}\)' || echo 0)"
> 
> I think git avoids \{m,n\} ranges where possible (for portability).
> This could be:
> 
> 	meld_major=${meld%%.*}
> 	meld_nonmajor=${meld#${meld_major}.}
> 	meld_minor=${meld_nonmajor%%.*}
> 
> or:
> 
> 	case $meld in
> 	[2-9].* | [1-9][0-9]* | 1.[5-9]* | 1.[1-9][0-9]*)	# >= 1.5.0
> 		meld_has_output_option=true ;;
> 	*)
> 		meld_has_output_option=false ;;
> 	esac
> 
> It's nice how self-contained this can be now that it's in its own
> file.  Thanks.

Right, I was using \{1,\} since that's what CodingStyle said to use instead of \+ (which I forgot to fixup above as you saw).

The case statement is nice and simple enough to understand. By doing meld --output /dev/null --help we're relying on older versions blowing up with --output and --help returning exit status 0. That seems pretty reasonable. The case statement does seem sufficient but just trying --output and seeing what happens is even simpler. Your example also uses $merge_tool_path, which I forgot to do, so I'll be sure to include that too.

I'll wait until tomorrow to see if there are any more comments and reroll.

Thanks,
-- 
					David
David Aguilar· Aug 19, 2011, 09:14 UTC · re: David Aguilar · lore

[PATCH v2 4/4] mergetools/meld: Use '--output' when available

meld 1.5.0 and newer allow the output file to be specified when merging multiple files. Check whether the meld command supports '--output' and use it when available.

Signed-off-by: David Aguilar <davvid@gmail.com>
---
 mergetools/meld |   25 ++++++++++++++++++++++++-
 1 files changed, 24 insertions(+), 1 deletions(-)
Changes since v1 with help from Jonathan Nieder:

Use $LOCAL $BASE REMOTE when calling meld. Avoid shell portability issues by using the return status. of --output instead of parsing --version.

Show changes to mergetools/meld +24 −1
diff --git a/mergetools/meld b/mergetools/meld
index 73d70ae..eaa115c 100644
--- a/mergetools/meld
+++ b/mergetools/meld
@@ -3,7 +3,30 @@ diff_cmd () {
 }
 
 merge_cmd () {
+	if test -z "${meld_has_output_option:+set}"
+	then
+		check_meld_for_output_version
+	fi
 	touch "$BACKUP"
-	"$merge_tool_path" "$LOCAL" "$MERGED" "$REMOTE"
+	if test "$meld_has_output_option" = true
+	then
+		"$merge_tool_path" --output "$MERGED" \
+			"$LOCAL" "$BASE" "$REMOTE"
+	else
+		"$merge_tool_path" "$LOCAL" "$MERGED" "$REMOTE"
+	fi
 	check_unchanged
 }
+
+# Check whether 'meld --output <file>' is supported
+check_meld_for_output_version () {
+	meld_path="$(git config mergetool.meld.path)"
+	meld_path="${meld_path:-meld}"
+
+	if "$meld_path" --output /dev/null --help >/dev/null 2>&1
+	then
+		meld_has_output_option=true
+	else
+		meld_has_output_option=false
+	fi
+}

-- 
1.7.6.553.g1899
Junio C Hamano· Aug 19, 2011, 22:58 UTC · re: David Aguilar · lore

Re: [PATCH v2 4/4] mergetools/meld: Use '--output' when available

David Aguilar <davvid@gmail.com> writes:
Show 10 quoted lines
> meld 1.5.0 and newer allow the output file to be specified
> when merging multiple files.  Check whether the meld command
> supports '--output' and use it when available.
>
> Signed-off-by: David Aguilar <davvid@gmail.com>
> ---
>  mergetools/meld |   25 ++++++++++++++++++++++++-
>  1 files changed, 24 insertions(+), 1 deletions(-)
>
> Changes since v1 with help from Jonathan Nieder:
Thanks, both of you. Queued all patches in the series.
Sebastian Schuberth· Aug 18, 2011, 07:57 UTC · re: David Aguilar · lore

Re: [PATCH 0/4] Refactor git-mergetool--lib.sh

On Thu, Aug 18, 2011 at 09:23, David Aguilar <davvid@gmail.com> wrote:
> This series splits the git-mergetool--lib.sh file into separate
> tool scriptlets that define the commands for diff and merge.

Very nice as it simplifies adding new merge tools to adding a single file with little code inside. Thanks!

-- 
Sebastian Schuberth

← back to recent threads