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

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

[David Aguilar]

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

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

[David Aguilar]

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

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

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

[David Aguilar]

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

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

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

[David Aguilar]

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

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

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

[David Aguilar]

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

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

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

[Sebastian Schuberth]

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

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

[Sebastian Schuberth]

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

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

[Jonathan Nieder]

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

> +	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.

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

[David Aguilar]

On Thu, Aug 18, 2011 at 09:49:33AM +0200, Sebastian Schuberth wrote:
> 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

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

[David Aguilar]

On Thu, Aug 18, 2011 at 03:13:10AM -0500, Jonathan Nieder wrote:
> 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.

> 
> > +	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...


> 	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

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

[Junio C Hamano]

David Aguilar <davvid@gmail.com> writes:

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

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

[David Aguilar]

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.

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

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

[Junio C Hamano]

David Aguilar <davvid@gmail.com> writes:

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

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

[Jonathan Nieder]

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
[...]
> @@ -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(-)

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

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

[Jonathan Nieder]

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.

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

[David Aguilar]

On Oct 9, 2011, at 4:43 AM, Jonathan Nieder <jrnieder@gmail.com> wrote:

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

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

[Jonathan Nieder]

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.

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

[Junio C Hamano]

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.