git-{diff,merge} refactor round 2

git-{diff,merge} refactor round 2

[David Aguilar]

Here's the 2nd round of refactoring.

This is based on Junio's pu branch.

I'm including the difftool.prompt patch in this series
because it is a dependency and including it here makes
that obvious.

I tried to keep the dependencies untangled while still being
able to manage the various command-line flags all in one
place.  Alas, this is shell so it can only be so elegant.

I went ahead and rolled in the "remove -o $MERGED" from
tkdiff for the diff mode case.


Still TODO:
incorporate the "add diffuse as a merge tool" patch.

Is there more that can be refactored?
Probably the part that sets up candidate mergetools,
replacing that with a function might be useful.

Here's what I've got so far.

[PATCH 01/10] difftool: add support for a difftool.prompt config variable

[David Aguilar]

difftool now supports difftool.prompt so that users do not have to
pass --no-prompt or hit enter each time a diff tool is launched.
The --prompt flag overrides the configuration variable.

Signed-off-by: David Aguilar <davvid@gmail.com>
---
 Documentation/config.txt       |    3 ++
 Documentation/git-difftool.txt |   10 +++++-
 git-difftool-helper.sh         |   10 +++++-
 git-difftool.perl              |   15 +++++++--
 t/t7800-difftool.sh            |   64 ++++++++++++++++++++++++++++++++++++++++
 5 files changed, 96 insertions(+), 6 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 185a9ef..79c8b66 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -684,6 +684,9 @@ difftool.<tool>.cmd::
 	is set to the name of the temporary file containing the contents
 	of the diff post-image.
 
+difftool.prompt::
+	Prompt before each invocation of the diff tool.
+
 diff.wordRegex::
 	A POSIX Extended Regular Expression used to determine what is a "word"
 	when performing word-by-word difference calculations.  Character
diff --git a/Documentation/git-difftool.txt b/Documentation/git-difftool.txt
index a00e943..73d4782 100644
--- a/Documentation/git-difftool.txt
+++ b/Documentation/git-difftool.txt
@@ -7,7 +7,7 @@ git-difftool - Show changes using common diff tools
 
 SYNOPSIS
 --------
-'git difftool' [--tool=<tool>] [-y|--no-prompt] [<'git diff' options>]
+'git difftool' [--tool=<tool>] [-y|--no-prompt|--prompt] [<'git diff' options>]
 
 DESCRIPTION
 -----------
@@ -21,6 +21,11 @@ OPTIONS
 --no-prompt::
 	Do not prompt before launching a diff tool.
 
+--prompt::
+	Prompt before each invocation of the diff tool.
+	This is the default behaviour; the option is provided to
+	override any configuration settings.
+
 -t <tool>::
 --tool=<tool>::
 	Use the diff tool specified by <tool>.
@@ -72,6 +77,9 @@ difftool.<tool>.cmd::
 +
 See the `--tool=<tool>` option above for more details.
 
+difftool.prompt::
+	Prompt before each invocation of the diff tool.
+
 SEE ALSO
 --------
 linkgit:git-diff[1]::
diff --git a/git-difftool-helper.sh b/git-difftool-helper.sh
index b91002b..02bb135 100755
--- a/git-difftool-helper.sh
+++ b/git-difftool-helper.sh
@@ -7,9 +7,15 @@
 #
 # Copyright (c) 2009 David Aguilar
 
-# Set GIT_DIFFTOOL_NO_PROMPT to bypass the per-file prompt.
+# difftool.prompt controls the default prompt/no-prompt behavior
+# and is overridden with $GIT_DIFFTOOL*_PROMPT.
 should_prompt () {
-	test -z "$GIT_DIFFTOOL_NO_PROMPT"
+	prompt=$(git config --bool difftool.prompt || echo true)
+	if test "$prompt" = true; then
+		test -z "$GIT_DIFFTOOL_NO_PROMPT"
+	else
+		test -n "$GIT_DIFFTOOL_PROMPT"
+	fi
 }
 
 # This function prepares temporary files and launches the appropriate
diff --git a/git-difftool.perl b/git-difftool.perl
index 8c160e5..985dfe0 100755
--- a/git-difftool.perl
+++ b/git-difftool.perl
@@ -2,9 +2,12 @@
 # Copyright (c) 2009 David Aguilar
 #
 # This is a wrapper around the GIT_EXTERNAL_DIFF-compatible
-# git-difftool-helper script.  This script exports
-# GIT_EXTERNAL_DIFF and GIT_PAGER for use by git, and
-# GIT_DIFFTOOL_NO_PROMPT and GIT_DIFF_TOOL for use by git-difftool-helper.
+# git-difftool-helper script.
+#
+# This script exports GIT_EXTERNAL_DIFF and GIT_PAGER for use by git.
+# GIT_DIFFTOOL_NO_PROMPT, GIT_DIFFTOOL_PROMPT, and GIT_DIFF_TOOL
+# are exported for use by git-difftool-helper.
+#
 # Any arguments that are unknown to this script are forwarded to 'git diff'.
 
 use strict;
@@ -62,6 +65,12 @@ sub generate_command
 		}
 		if ($arg eq '-y' || $arg eq '--no-prompt') {
 			$ENV{GIT_DIFFTOOL_NO_PROMPT} = 'true';
+			delete $ENV{GIT_DIFFTOOL_PROMPT};
+			next;
+		}
+		if ($arg eq '--prompt') {
+			$ENV{GIT_DIFFTOOL_PROMPT} = 'true';
+			delete $ENV{GIT_DIFFTOOL_NO_PROMPT};
 			next;
 		}
 		if ($arg eq '-h' || $arg eq '--help') {
diff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh
index ceef84b..33d07e6 100755
--- a/t/t7800-difftool.sh
+++ b/t/t7800-difftool.sh
@@ -15,6 +15,7 @@ remove_config_vars()
 	# Unset all config variables used by git-difftool
 	git config --unset diff.tool
 	git config --unset difftool.test-tool.cmd
+	git config --unset difftool.prompt
 	git config --unset merge.tool
 	git config --unset mergetool.test-tool.cmd
 	return 0
@@ -26,11 +27,18 @@ restore_test_defaults()
 	remove_config_vars
 	unset GIT_DIFF_TOOL
 	unset GIT_MERGE_TOOL
+	unset GIT_DIFFTOOL_PROMPT
 	unset GIT_DIFFTOOL_NO_PROMPT
 	git config diff.tool test-tool &&
 	git config difftool.test-tool.cmd 'cat $LOCAL'
 }
 
+prompt_given()
+{
+	prompt="$1"
+	test "$prompt" = "Hit return to launch 'test-tool': branch"
+}
+
 # Create a file on master and change it on branch
 test_expect_success 'setup' '
 	echo master >file &&
@@ -116,6 +124,62 @@ test_expect_success 'GIT_DIFFTOOL_NO_PROMPT variable' '
 	restore_test_defaults
 '
 
+# git-difftool supports the difftool.prompt variable.
+# Test that GIT_DIFFTOOL_PROMPT can override difftool.prompt = false
+test_expect_success 'GIT_DIFFTOOL_PROMPT variable' '
+	git config difftool.prompt false &&
+	GIT_DIFFTOOL_PROMPT=true &&
+	export GIT_DIFFTOOL_PROMPT &&
+
+	prompt=$(echo | git difftool --prompt branch | tail -1) &&
+	prompt_given "$prompt" &&
+
+	restore_test_defaults
+'
+
+# Test that we don't have to pass --no-prompt when difftool.prompt is false
+test_expect_success 'difftool.prompt config variable is false' '
+	git config difftool.prompt false &&
+
+	diff=$(git difftool branch) &&
+	test "$diff" = "branch" &&
+
+	restore_test_defaults
+'
+
+# Test that the -y flag can override difftool.prompt = true
+test_expect_success 'difftool.prompt can overridden with -y' '
+	git config difftool.prompt true &&
+
+	diff=$(git difftool -y branch) &&
+	test "$diff" = "branch" &&
+
+	restore_test_defaults
+'
+
+# Test that the --prompt flag can override difftool.prompt = false
+test_expect_success 'difftool.prompt can overridden with --prompt' '
+	git config difftool.prompt false &&
+
+	prompt=$(echo | git difftool --prompt branch | tail -1) &&
+	prompt_given "$prompt" &&
+
+	restore_test_defaults
+'
+
+# Test that the last flag passed on the command-line wins
+test_expect_success 'difftool last flag wins' '
+	diff=$(git difftool --prompt --no-prompt branch) &&
+	test "$diff" = "branch" &&
+
+	restore_test_defaults &&
+
+	prompt=$(echo | git difftool --no-prompt --prompt branch | tail -1) &&
+	prompt_given "$prompt" &&
+
+	restore_test_defaults
+'
+
 # git-difftool falls back to git-mergetool config variables
 # so test that behavior here
 test_expect_success 'difftool + mergetool config variables' '
-- 
1.6.1.3

[PATCH 02/10] mergetool: use $( ... ) instead of `backticks`

[David Aguilar]

This makes mergetool consistent with Documentation/CodingGuidelines.

Signed-off-by: David Aguilar <davvid@gmail.com>
---
 git-mergetool.sh |   16 ++++++++--------
 1 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/git-mergetool.sh b/git-mergetool.sh
index 87fa88a..7c04031 100755
--- a/git-mergetool.sh
+++ b/git-mergetool.sh
@@ -137,7 +137,7 @@ checkout_staged_file () {
 merge_file () {
     MERGED="$1"
 
-    f=`git ls-files -u -- "$MERGED"`
+    f=$(git ls-files -u -- "$MERGED")
     if test -z "$f" ; then
 	if test ! -f "$MERGED" ; then
 	    echo "$MERGED: file not found"
@@ -156,9 +156,9 @@ merge_file () {
     mv -- "$MERGED" "$BACKUP"
     cp -- "$BACKUP" "$MERGED"
 
-    base_mode=`git ls-files -u -- "$MERGED" | awk '{if ($3==1) print $1;}'`
-    local_mode=`git ls-files -u -- "$MERGED" | awk '{if ($3==2) print $1;}'`
-    remote_mode=`git ls-files -u -- "$MERGED" | awk '{if ($3==3) print $1;}'`
+    base_mode=$(git ls-files -u -- "$MERGED" | awk '{if ($3==1) print $1;}')
+    local_mode=$(git ls-files -u -- "$MERGED" | awk '{if ($3==2) print $1;}')
+    remote_mode=$(git ls-files -u -- "$MERGED" | awk '{if ($3==3) print $1;}')
 
     base_present   && checkout_staged_file 1 "$MERGED" "$BASE"
     local_present  && checkout_staged_file 2 "$MERGED" "$LOCAL"
@@ -308,7 +308,7 @@ do
 	-t|--tool*)
 	    case "$#,$1" in
 		*,*=*)
-		    merge_tool=`expr "z$1" : 'z-[^=]*=\(.*\)'`
+		    merge_tool=$(expr "z$1" : 'z-[^=]*=\(.*\)')
 		    ;;
 		1,*)
 		    usage ;;
@@ -356,7 +356,7 @@ valid_tool() {
 }
 
 init_merge_tool_path() {
-	merge_tool_path=`git config mergetool.$1.path`
+	merge_tool_path=$(git config mergetool.$1.path)
 	if test -z "$merge_tool_path" ; then
 		case "$1" in
 			emerge)
@@ -387,7 +387,7 @@ prompt_after_failed_merge() {
 }
 
 if test -z "$merge_tool"; then
-    merge_tool=`git config merge.tool`
+    merge_tool=$(git config merge.tool)
     if test -n "$merge_tool" && ! valid_tool "$merge_tool"; then
 	    echo >&2 "git config option merge.tool set to unknown tool: $merge_tool"
 	    echo >&2 "Resetting to default..."
@@ -447,7 +447,7 @@ last_status=0
 rollup_status=0
 
 if test $# -eq 0 ; then
-    files=`git ls-files -u | sed -e 's/^[^	]*	//' | sort -u`
+    files=$(git ls-files -u | sed -e 's/^[^	]*	//' | sort -u)
     if test -z "$files" ; then
 	echo "No files need merging"
 	exit 0
-- 
1.6.1.3

[PATCH 03/10] Add a mergetool-lib scriptlet for holding common merge tool functions

[David Aguilar]

git-mergetool-lib provides common merge tool functions.

Signed-off-by: David Aguilar <davvid@gmail.com>
---
 .gitignore                          |    1 +
 Documentation/git-mergetool-lib.txt |   42 +++++++++++++++++++++++++++++++++++
 Makefile                            |    1 +
 command-list.txt                    |    1 +
 git-mergetool-lib.sh                |   41 ++++++++++++++++++++++++++++++++++
 5 files changed, 86 insertions(+), 0 deletions(-)
 create mode 100644 Documentation/git-mergetool-lib.txt
 create mode 100644 git-mergetool-lib.sh

diff --git a/.gitignore b/.gitignore
index 966c886..75c154a 100644
--- a/.gitignore
+++ b/.gitignore
@@ -80,6 +80,7 @@ git-merge-recursive
 git-merge-resolve
 git-merge-subtree
 git-mergetool
+git-mergetool-lib
 git-mktag
 git-mktree
 git-name-rev
diff --git a/Documentation/git-mergetool-lib.txt b/Documentation/git-mergetool-lib.txt
new file mode 100644
index 0000000..a8d62f5
--- /dev/null
+++ b/Documentation/git-mergetool-lib.txt
@@ -0,0 +1,42 @@
+git-mergetool-lib(1)
+====================
+
+NAME
+----
+git-mergetool-lib - Common git merge tool shell scriptlets
+
+SYNOPSIS
+--------
+'. "$(git --exec-path)/git-mergetool-lib"'
+
+DESCRIPTION
+-----------
+
+This is not a command the end user would want to run.  Ever.
+This documentation is meant for people who are studying the
+Porcelain-ish scripts and/or are writing new ones.
+
+The 'git-mergetool-lib' scriptlet is designed to be sourced (using
+`.`) by other shell scripts to set up functions for working
+with git merge tools.
+
+Before sourcing it, your script should set up a few variables;
+`TOOL_MODE` is used to define the operation mode for various
+functions.  'diff' and 'merge' are valid values.
+
+FUNCTIONS
+---------
+get_merge_tool_path::
+	returns the `merge_tool_path` for a `merge_tool`.
+
+Author
+------
+Written by David Aguilar <davvid@gmail.com>
+
+Documentation
+--------------
+Documentation by David Aguilar and the git-list <git@vger.kernel.org>.
+
+GIT
+---
+Part of the linkgit:git[1] suite
diff --git a/Makefile b/Makefile
index d77fd71..086f9e7 100644
--- a/Makefile
+++ b/Makefile
@@ -284,6 +284,7 @@ SCRIPT_SH += git-merge-octopus.sh
 SCRIPT_SH += git-merge-one-file.sh
 SCRIPT_SH += git-merge-resolve.sh
 SCRIPT_SH += git-mergetool.sh
+SCRIPT_SH += git-mergetool-lib.sh
 SCRIPT_SH += git-parse-remote.sh
 SCRIPT_SH += git-pull.sh
 SCRIPT_SH += git-quiltimport.sh
diff --git a/command-list.txt b/command-list.txt
index fb03a2e..922c815 100644
--- a/command-list.txt
+++ b/command-list.txt
@@ -69,6 +69,7 @@ git-merge-file                          plumbingmanipulators
 git-merge-index                         plumbingmanipulators
 git-merge-one-file                      purehelpers
 git-mergetool                           ancillarymanipulators
+git-mergetool-lib                       purehelpers
 git-merge-tree                          ancillaryinterrogators
 git-mktag                               plumbingmanipulators
 git-mktree                              plumbingmanipulators
diff --git a/git-mergetool-lib.sh b/git-mergetool-lib.sh
new file mode 100644
index 0000000..c307a5d
--- /dev/null
+++ b/git-mergetool-lib.sh
@@ -0,0 +1,41 @@
+# git-mergetool-lib is a library for common merge tool functions
+diff_mode() {
+	test $TOOL_MODE = "diff"
+}
+
+get_merge_tool_path () {
+	if test -z "$2"; then
+		case "$1" in
+		emerge)
+			path=emacs
+			;;
+		*)
+			path="$1"
+			;;
+		esac
+	fi
+	echo "$path"
+}
+
+valid_tool () {
+	case "$1" in
+	kdiff3 | kompare | tkdiff | xxdiff | meld | opendiff | emerge | vimdiff | gvimdiff | ecmerge)
+		if test "$1" = "kompare" && ! diff_mode; then
+			return 1
+		fi
+		;; # happy
+	*)
+		if ! test -n "$(get_custom_cmd "$1")"; then
+			return 1
+		fi ;;
+	esac
+}
+
+get_custom_cmd () {
+	diff_mode &&
+	custom_cmd="$(git config difftool.$1.cmd)"
+	test -z "$custom_cmd" &&
+	custom_cmd="$(git config mergetool.$1.cmd)"
+	test -n "$custom_cmd" &&
+	echo "$custom_cmd"
+}
-- 
1.6.1.3

[PATCH 04/10] mergetool: use get_mergetool_path from git-mergetool-lib

[David Aguilar]

This refactors git-mergetool to use get_mergetool_path().

Signed-off-by: David Aguilar <davvid@gmail.com>
---
 git-mergetool.sh |   20 +++-----------------
 1 files changed, 3 insertions(+), 17 deletions(-)

diff --git a/git-mergetool.sh b/git-mergetool.sh
index 7c04031..732a5b7 100755
--- a/git-mergetool.sh
+++ b/git-mergetool.sh
@@ -12,6 +12,7 @@ USAGE='[--tool=tool] [-y|--no-prompt|--prompt] [file to merge] ...'
 SUBDIRECTORY_OK=Yes
 OPTIONS_SPEC=
 . git-sh-setup
+. git-mergetool-lib
 require_work_tree
 
 # Returns true if the mode reflects a symlink
@@ -355,20 +356,6 @@ valid_tool() {
 	esac
 }
 
-init_merge_tool_path() {
-	merge_tool_path=$(git config mergetool.$1.path)
-	if test -z "$merge_tool_path" ; then
-		case "$1" in
-			emerge)
-				merge_tool_path=emacs
-				;;
-			*)
-				merge_tool_path=$1
-				;;
-		esac
-	fi
-}
-
 prompt_after_failed_merge() {
     while true; do
 	printf "Continue merging other unresolved paths (y/n) ? "
@@ -412,7 +399,7 @@ if test -z "$merge_tool" ; then
     fi
     echo "merge tool candidates: $merge_tool_candidates"
     for i in $merge_tool_candidates; do
-        init_merge_tool_path $i
+        merge_tool_path="$(get_merge_tool_path "$i" "$merge_tool_path")"
         if type "$merge_tool_path" > /dev/null 2>&1; then
             merge_tool=$i
             break
@@ -428,8 +415,7 @@ else
         exit 1
     fi
 
-    init_merge_tool_path "$merge_tool"
-
+    merge_tool_path="$(get_merge_tool_path "$merge_tool")"
     merge_keep_backup="$(git config --bool merge.keepBackup || echo true)"
     merge_keep_temporaries="$(git config --bool mergetool.keepTemporaries || echo false)"
 
-- 
1.6.1.3

[PATCH 05/10] difftool: use get_mergetool_path from git-mergetool-lib

[David Aguilar]

This refactors git-difftool-helper to use get_mergetool_path().

Signed-off-by: David Aguilar <davvid@gmail.com>
---
 git-difftool-helper.sh |   33 ++++++++++++---------------------
 1 files changed, 12 insertions(+), 21 deletions(-)

diff --git a/git-difftool-helper.sh b/git-difftool-helper.sh
index 02bb135..d1bea18 100755
--- a/git-difftool-helper.sh
+++ b/git-difftool-helper.sh
@@ -7,6 +7,9 @@
 #
 # Copyright (c) 2009 David Aguilar
 
+# Load common functions from git-mergetool-lib
+. git-mergetool-lib
+
 # difftool.prompt controls the default prompt/no-prompt behavior
 # and is overridden with $GIT_DIFFTOOL*_PROMPT.
 should_prompt () {
@@ -125,25 +128,6 @@ valid_tool() {
 	esac
 }
 
-# Sets up the merge_tool_path variable.
-# This handles the difftool.<tool>.path configuration.
-# This also falls back to mergetool defaults.
-init_merge_tool_path() {
-	merge_tool_path=$(git config difftool."$1".path)
-	test -z "$merge_tool_path" &&
-	merge_tool_path=$(git config mergetool."$1".path)
-	if test -z "$merge_tool_path"; then
-		case "$1" in
-		emerge)
-			merge_tool_path=emacs
-			;;
-		*)
-			merge_tool_path="$1"
-			;;
-		esac
-	fi
-}
-
 # Allow GIT_DIFF_TOOL and GIT_MERGE_TOOL to provide default values
 test -n "$GIT_MERGE_TOOL" && merge_tool="$GIT_MERGE_TOOL"
 test -n "$GIT_DIFF_TOOL" && merge_tool="$GIT_DIFF_TOOL"
@@ -187,7 +171,7 @@ if test -z "$merge_tool"; then
 	# Loop over each candidate and stop when a valid merge tool is found.
 	for i in $merge_tool_candidates
 	do
-		init_merge_tool_path $i
+		merge_tool_path="$(get_merge_tool_path "$i")"
 		if type "$merge_tool_path" > /dev/null 2>&1; then
 			merge_tool=$i
 			break
@@ -206,7 +190,14 @@ else
 		exit 1
 	fi
 
-	init_merge_tool_path "$merge_tool"
+	# Sets up the merge_tool_path variable.
+	# This handles the difftool.<tool>.path configuration variable
+	# and falls back to mergetool defaults.
+	merge_tool_path=$(git config difftool."$1".path)
+	test -z "$merge_tool_path" &&
+	merge_tool_path=$(git config mergetool."$1".path)
+	merge_tool_path="$(get_merge_tool_path "$merge_tool" \
+	                                       "$merge_tool_path")"
 
 	if test -z "$merge_tool_cmd" && ! type "$merge_tool_path" > /dev/null 2>&1; then
 		echo "The merge tool $merge_tool is not available as '$merge_tool_path'"
-- 
1.6.1.3

[PATCH 06/10] mergetool: use valid_tool from git-mergetool-lib

[David Aguilar]

This refactors git-mergetool to use valid_tool from git-mergetool-lib.

Signed-off-by: David Aguilar <davvid@gmail.com>
---
 git-mergetool.sh |   21 +++------------------
 1 files changed, 3 insertions(+), 18 deletions(-)

diff --git a/git-mergetool.sh b/git-mergetool.sh
index 732a5b7..957993c 100755
--- a/git-mergetool.sh
+++ b/git-mergetool.sh
@@ -11,6 +11,7 @@
 USAGE='[--tool=tool] [-y|--no-prompt|--prompt] [file to merge] ...'
 SUBDIRECTORY_OK=Yes
 OPTIONS_SPEC=
+TOOL_MODE=merge
 . git-sh-setup
 . git-mergetool-lib
 require_work_tree
@@ -338,24 +339,6 @@ do
     shift
 done
 
-valid_custom_tool()
-{
-    merge_tool_cmd="$(git config mergetool.$1.cmd)"
-    test -n "$merge_tool_cmd"
-}
-
-valid_tool() {
-	case "$1" in
-		kdiff3 | tkdiff | xxdiff | meld | opendiff | emerge | vimdiff | gvimdiff | ecmerge)
-			;; # happy
-		*)
-			if ! valid_custom_tool "$1"; then
-				return 1
-			fi
-			;;
-	esac
-}
-
 prompt_after_failed_merge() {
     while true; do
 	printf "Continue merging other unresolved paths (y/n) ? "
@@ -409,12 +392,14 @@ if test -z "$merge_tool" ; then
 	echo "No known merge resolution program available."
 	exit 1
     fi
+    merge_tool_cmd="$(git config mergetool."$merge_tool".cmd)"
 else
     if ! valid_tool "$merge_tool"; then
         echo >&2 "Unknown merge_tool $merge_tool"
         exit 1
     fi
 
+    merge_tool_cmd="$(git config mergetool.$merge_tool.cmd)"
     merge_tool_path="$(get_merge_tool_path "$merge_tool")"
     merge_keep_backup="$(git config --bool merge.keepBackup || echo true)"
     merge_keep_temporaries="$(git config --bool mergetool.keepTemporaries || echo false)"
-- 
1.6.1.3

[PATCH 07/10] difftool: use valid_tool from git-mergetool-lib

[David Aguilar]

This refactors difftool to use valid_tool fro git-mergetool-lib

Signed-off-by: David Aguilar <davvid@gmail.com>
---
 git-difftool-helper.sh |   29 +++--------------------------
 1 files changed, 3 insertions(+), 26 deletions(-)

diff --git a/git-difftool-helper.sh b/git-difftool-helper.sh
index d1bea18..6cc5ab5 100755
--- a/git-difftool-helper.sh
+++ b/git-difftool-helper.sh
@@ -8,6 +8,7 @@
 # Copyright (c) 2009 David Aguilar
 
 # Load common functions from git-mergetool-lib
+TOOL_MODE=diff
 . git-mergetool-lib
 
 # difftool.prompt controls the default prompt/no-prompt behavior
@@ -105,29 +106,6 @@ launch_merge_tool () {
 	esac
 }
 
-# Verifies that (difftool|mergetool).<tool>.cmd exists
-valid_custom_tool() {
-	merge_tool_cmd="$(git config difftool.$1.cmd)"
-	test -z "$merge_tool_cmd" &&
-	merge_tool_cmd="$(git config mergetool.$1.cmd)"
-	test -n "$merge_tool_cmd"
-}
-
-# Verifies that the chosen merge tool is properly setup.
-# Built-in merge tools are always valid.
-valid_tool() {
-	case "$1" in
-	kdiff3 | kompare | tkdiff | xxdiff | meld | opendiff | emerge | vimdiff | gvimdiff | ecmerge)
-		;; # happy
-	*)
-		if ! valid_custom_tool "$1"
-		then
-			return 1
-		fi
-		;;
-	esac
-}
-
 # Allow GIT_DIFF_TOOL and GIT_MERGE_TOOL to provide default values
 test -n "$GIT_MERGE_TOOL" && merge_tool="$GIT_MERGE_TOOL"
 test -n "$GIT_DIFF_TOOL" && merge_tool="$GIT_DIFF_TOOL"
@@ -183,6 +161,7 @@ if test -z "$merge_tool"; then
 		exit 1
 	fi
 
+	merge_tool_cmd=$(get_custom_cmd "$merge_tool")
 else
 	# A merge tool has been set, so verify that it's valid.
 	if ! valid_tool "$merge_tool"; then
@@ -190,9 +169,7 @@ else
 		exit 1
 	fi
 
-	# Sets up the merge_tool_path variable.
-	# This handles the difftool.<tool>.path configuration variable
-	# and falls back to mergetool defaults.
+	merge_tool_cmd=$(get_custom_cmd "$merge_tool")
 	merge_tool_path=$(git config difftool."$1".path)
 	test -z "$merge_tool_path" &&
 	merge_tool_path=$(git config mergetool."$1".path)
-- 
1.6.1.3

[PATCH 08/10] mergetool-lib: introduce run_mergetool

[David Aguilar]

run_mergetool contains the common functionality for launching
mergetools.  There's some context-specific behavior but overall
it's better to have all of this stuff in one place versus multiple
places.

Signed-off-by: David Aguilar <davvid@gmail.com>
---
 Documentation/git-mergetool-lib.txt |    4 +
 git-mergetool-lib.sh                |  141 +++++++++++++++++++++++++++++++++++
 2 files changed, 145 insertions(+), 0 deletions(-)

diff --git a/Documentation/git-mergetool-lib.txt b/Documentation/git-mergetool-lib.txt
index a8d62f5..3060673 100644
--- a/Documentation/git-mergetool-lib.txt
+++ b/Documentation/git-mergetool-lib.txt
@@ -29,6 +29,10 @@ FUNCTIONS
 get_merge_tool_path::
 	returns the `merge_tool_path` for a `merge_tool`.
 
+run_mergetool::
+	launches a merge tool given the tool name and a true/false
+	flag to indicate whether a merge base is present
+
 Author
 ------
 Written by David Aguilar <davvid@gmail.com>
diff --git a/git-mergetool-lib.sh b/git-mergetool-lib.sh
index c307a5d..9c26b5f 100644
--- a/git-mergetool-lib.sh
+++ b/git-mergetool-lib.sh
@@ -3,7 +3,12 @@ diff_mode() {
 	test $TOOL_MODE = "diff"
 }
 
+merge_mode() {
+	test $TOOL_MODE = "merge"
+}
+
 get_merge_tool_path () {
+	path="$1"
 	if test -z "$2"; then
 		case "$1" in
 		emerge)
@@ -17,6 +22,11 @@ get_merge_tool_path () {
 	echo "$path"
 }
 
+# Overridden in git-mergetool
+check_unchanged () {
+	true
+}
+
 valid_tool () {
 	case "$1" in
 	kdiff3 | kompare | tkdiff | xxdiff | meld | opendiff | emerge | vimdiff | gvimdiff | ecmerge)
@@ -39,3 +49,134 @@ get_custom_cmd () {
 	test -n "$custom_cmd" &&
 	echo "$custom_cmd"
 }
+
+run_mergetool () {
+	base_present="$2"
+	if diff_mode; then
+		base_present="false"
+	fi
+	if test -z "$base_present"; then
+		base_present="true"
+	fi
+
+	case "$1" in
+	kdiff3)
+		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=$?
+		;;
+	kompare)
+		"$merge_tool_path" "$LOCAL" "$REMOTE"
+		status=$?
+		;;
+	tkdiff)
+		if $base_present; then
+			"$merge_tool_path" -a "$BASE" -o "$MERGED" "$LOCAL" "$REMOTE"
+		else
+			if diff_mode; then
+				"$merge_tool_path" "$LOCAL" "$REMOTE"
+			else
+				"$merge_tool_path" -o "$MERGED" "$LOCAL" "$REMOTE"
+			fi
+		fi
+		status=$?
+		;;
+	meld)
+		merge_mode && touch "$BACKUP"
+		if merge_mode; then
+			"$merge_tool_path" "$LOCAL" "$MERGED" "$REMOTE"
+		else
+			"$merge_tool_path" "$LOCAL" "$REMOTE"
+		fi
+		check_unchanged
+		;;
+	vimdiff)
+		if merge_mode; then
+			touch "$BACKUP"
+			"$merge_tool_path" -c "wincmd l" "$LOCAL" "$MERGED" "$REMOTE"
+			check_unchanged
+		else
+			"$merge_tool_path" -c "wincmd l" "$LOCAL" "$REMOTE"
+		fi
+		;;
+	gvimdiff)
+		if merge_mode; then
+			touch "$BACKUP"
+			"$merge_tool_path" -c "wincmd l" -f "$LOCAL" "$MERGED" "$REMOTE"
+			check_unchanged
+		else
+			"$merge_tool_path" -c "wincmd l" -f "$LOCAL" "$REMOTE"
+		fi
+		;;
+	xxdiff)
+		merge_mode && 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_mode && extra=--show-merged-pane
+			"$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
+		;;
+	opendiff)
+		merge_mode && 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
+		;;
+	ecmerge)
+		merge_mode && 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
+		;;
+	emerge)
+		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=$?
+		;;
+	*)
+		if test -n "$merge_tool_cmd"; then
+			if merge_mode &&
+			test "$merge_tool_trust_exit_code" = "false"; then
+				touch "$BACKUP"
+				( eval $merge_tool_cmd )
+				check_unchanged
+			else
+				( eval $merge_tool_cmd )
+				status=$?
+			fi
+		fi
+		;;
+	esac
+}
-- 
1.6.1.3

[PATCH 09/10] difftool: use run_mergetool from git-mergetool-lib

[David Aguilar]

This refactors git-mergetool-helper to use run_mergetool.

Signed-off-by: David Aguilar <davvid@gmail.com>
---
 git-difftool-helper.sh |   62 +-----------------------------------------------
 1 files changed, 1 insertions(+), 61 deletions(-)

diff --git a/git-difftool-helper.sh b/git-difftool-helper.sh
index 6cc5ab5..2dfc870 100755
--- a/git-difftool-helper.sh
+++ b/git-difftool-helper.sh
@@ -43,67 +43,7 @@ launch_merge_tool () {
 	fi
 
 	# Run the appropriate merge tool command
-	case "$merge_tool" in
-	kdiff3)
-		basename=$(basename "$MERGED")
-		"$merge_tool_path" --auto \
-			--L1 "$basename (A)" \
-			--L2 "$basename (B)" \
-			-o "$MERGED" "$LOCAL" "$REMOTE" \
-			> /dev/null 2>&1
-		;;
-
-	kompare)
-		"$merge_tool_path" "$LOCAL" "$REMOTE"
-		;;
-
-	tkdiff)
-		"$merge_tool_path" -o "$MERGED" "$LOCAL" "$REMOTE"
-		;;
-
-	meld)
-		"$merge_tool_path" "$LOCAL" "$REMOTE"
-		;;
-
-	vimdiff)
-		"$merge_tool_path" -c "wincmd l" "$LOCAL" "$REMOTE"
-		;;
-
-	gvimdiff)
-		"$merge_tool_path" -c "wincmd l" -f "$LOCAL" "$REMOTE"
-		;;
-
-	xxdiff)
-		"$merge_tool_path" \
-			-X \
-			-R 'Accel.SaveAsMerged: "Ctrl-S"' \
-			-R 'Accel.Search: "Ctrl+F"' \
-			-R 'Accel.SearchForward: "Ctrl-G"' \
-			--merged-file "$MERGED" \
-			"$LOCAL" "$REMOTE"
-		;;
-
-	opendiff)
-		"$merge_tool_path" "$LOCAL" "$REMOTE" \
-			-merge "$MERGED" | cat
-		;;
-
-	ecmerge)
-		"$merge_tool_path" "$LOCAL" "$REMOTE" \
-			--default --mode=merge2 --to="$MERGED"
-		;;
-
-	emerge)
-		"$merge_tool_path" -f emerge-files-command \
-			"$LOCAL" "$REMOTE" "$(basename "$MERGED")"
-		;;
-
-	*)
-		if test -n "$merge_tool_cmd"; then
-			( eval $merge_tool_cmd )
-		fi
-		;;
-	esac
+	run_mergetool "$merge_tool";
 }
 
 # Allow GIT_DIFF_TOOL and GIT_MERGE_TOOL to provide default values
-- 
1.6.1.3

[PATCH 10/10] mergetool: use run_mergetool from git-mergetool-lib

[David Aguilar]

This refactors git-mergetool to use run_mergetool.

Signed-off-by: David Aguilar <davvid@gmail.com>
---
 git-mergetool.sh |   96 +++--------------------------------------------------
 1 files changed, 6 insertions(+), 90 deletions(-)

diff --git a/git-mergetool.sh b/git-mergetool.sh
index 957993c..2c6b325 100755
--- a/git-mergetool.sh
+++ b/git-mergetool.sh
@@ -190,96 +190,12 @@ merge_file () {
 	read ans
     fi
 
-    case "$merge_tool" in
-	kdiff3)
-	    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=$?
-	    ;;
-	tkdiff)
-	    if base_present ; then
-		"$merge_tool_path" -a "$BASE" -o "$MERGED" "$LOCAL" "$REMOTE"
-	    else
-		"$merge_tool_path" -o "$MERGED" "$LOCAL" "$REMOTE"
-	    fi
-	    status=$?
-	    ;;
-	meld)
-	    touch "$BACKUP"
-	    "$merge_tool_path" "$LOCAL" "$MERGED" "$REMOTE"
-	    check_unchanged
-	    ;;
-	vimdiff)
-	    touch "$BACKUP"
-	    "$merge_tool_path" -c "wincmd l" "$LOCAL" "$MERGED" "$REMOTE"
-	    check_unchanged
-	    ;;
-	gvimdiff)
-	    touch "$BACKUP"
-	    "$merge_tool_path" -c "wincmd l" -f "$LOCAL" "$MERGED" "$REMOTE"
-	    check_unchanged
-	    ;;
-	xxdiff)
-	    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 --show-merged-pane \
-		    -R 'Accel.SaveAsMerged: "Ctrl-S"' \
-		    -R 'Accel.Search: "Ctrl+F"' \
-		    -R 'Accel.SearchForward: "Ctrl-G"' \
-		    --merged-file "$MERGED" "$LOCAL" "$REMOTE"
-	    fi
-	    check_unchanged
-	    ;;
-	opendiff)
-	    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
-	    ;;
-	ecmerge)
-	    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
-	    ;;
-	emerge)
-	    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=$?
-	    ;;
-	*)
-	    if test -n "$merge_tool_cmd"; then
-		if test "$merge_tool_trust_exit_code" = "false"; then
-		    touch "$BACKUP"
-		    ( eval $merge_tool_cmd )
-		    check_unchanged
-		else
-		    ( eval $merge_tool_cmd )
-		    status=$?
-		fi
-	    fi
-	    ;;
-    esac
+    present=false
+    base_present &&
+    present=true
+
+    run_mergetool "$merge_tool" "$present"
+    status=$?
     if test "$status" -ne 0; then
 	echo "merge of $MERGED failed" 1>&2
 	mv -- "$BACKUP" "$MERGED"
-- 
1.6.1.3

Re: [PATCH 03/10] Add a mergetool-lib scriptlet for holding common merge tool functions

[Markus Heidelberg]

David Aguilar, 01.04.2009:
> 
> diff --git a/git-mergetool-lib.sh b/git-mergetool-lib.sh
> +valid_tool () {
> +	case "$1" in
> +	kdiff3 | kompare | tkdiff | xxdiff | meld | opendiff | emerge | vimdiff | gvimdiff | ecmerge)
> +		if test "$1" = "kompare" && ! diff_mode; then
> +			return 1
> +		fi
> +		;; # happy
> +	*)
> +		if ! test -n "$(get_custom_cmd "$1")"; then

Better this?
		if test -z "$(get_custom_cmd "$1")"; then

> +			return 1
> +		fi ;;

For consistency:
		fi
		;;

Re: [PATCH 08/10] mergetool-lib: introduce run_mergetool

[Markus Heidelberg]

David Aguilar, 01.04.2009:
> diff --git a/git-mergetool-lib.sh b/git-mergetool-lib.sh
> +run_mergetool () {
> +	base_present="$2"
> +	if diff_mode; then
> +		base_present="false"
> +	fi
> +	if test -z "$base_present"; then
> +		base_present="true"
> +	fi
> +
> +	case "$1" in
> +	kdiff3)
> +		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=$?
> +		;;
> +	kompare)
> +		"$merge_tool_path" "$LOCAL" "$REMOTE"
> +		status=$?
> +		;;
> +	tkdiff)
> +		if $base_present; then
> +			"$merge_tool_path" -a "$BASE" -o "$MERGED" "$LOCAL" "$REMOTE"
> +		else
> +			if diff_mode; then

Query merge_mode instead of diff_mode as in all the other places.

> +				"$merge_tool_path" "$LOCAL" "$REMOTE"
> +			else
> +				"$merge_tool_path" -o "$MERGED" "$LOCAL" "$REMOTE"
> +			fi
> +		fi
> +		status=$?
> +		;;
> +	meld)
> +		merge_mode && touch "$BACKUP"
> +		if merge_mode; then
> +			"$merge_tool_path" "$LOCAL" "$MERGED" "$REMOTE"

Put the touch command into the if then block as in the other places.

> +		else
> +			"$merge_tool_path" "$LOCAL" "$REMOTE"
> +		fi
> +		check_unchanged
> +		;;
> +	vimdiff)
> +		if merge_mode; then
> +			touch "$BACKUP"
> +			"$merge_tool_path" -c "wincmd l" "$LOCAL" "$MERGED" "$REMOTE"
> +			check_unchanged
> +		else
> +			"$merge_tool_path" -c "wincmd l" "$LOCAL" "$REMOTE"
> +		fi
> +		;;
> +	gvimdiff)
> +		if merge_mode; then
> +			touch "$BACKUP"
> +			"$merge_tool_path" -c "wincmd l" -f "$LOCAL" "$MERGED" "$REMOTE"
> +			check_unchanged
> +		else
> +			"$merge_tool_path" -c "wincmd l" -f "$LOCAL" "$REMOTE"
> +		fi
> +		;;
> +	xxdiff)
> +		merge_mode && 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_mode && extra=--show-merged-pane
> +			"$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
> +		;;
> +	opendiff)
> +		merge_mode && 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
> +		;;
> +	ecmerge)
> +		merge_mode && 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
> +		;;
> +	emerge)
> +		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=$?
> +		;;
> +	*)
> +		if test -n "$merge_tool_cmd"; then
> +			if merge_mode &&
> +			test "$merge_tool_trust_exit_code" = "false"; then
> +				touch "$BACKUP"
> +				( eval $merge_tool_cmd )
> +				check_unchanged
> +			else
> +				( eval $merge_tool_cmd )
> +				status=$?
> +			fi
> +		fi
> +		;;
> +	esac
> +}

Re: [PATCH 10/10] mergetool: use run_mergetool from git-mergetool-lib

[Markus Heidelberg]

David Aguilar, 01.04.2009:
> This refactors git-mergetool to use run_mergetool.
> 
> Signed-off-by: David Aguilar <davvid@gmail.com>
> ---
>  git-mergetool.sh |   96 +++--------------------------------------------------
>  1 files changed, 6 insertions(+), 90 deletions(-)
> 
> diff --git a/git-mergetool.sh b/git-mergetool.sh
> index 957993c..2c6b325 100755
> --- a/git-mergetool.sh
> +++ b/git-mergetool.sh
> @@ -190,96 +190,12 @@ merge_file () {
>  	read ans
>      fi
>  
> -    case "$merge_tool" in
> -	kdiff3)
> -	    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=$?
> -	    ;;
> -	tkdiff)
> -	    if base_present ; then
> -		"$merge_tool_path" -a "$BASE" -o "$MERGED" "$LOCAL" "$REMOTE"
> -	    else
> -		"$merge_tool_path" -o "$MERGED" "$LOCAL" "$REMOTE"
> -	    fi
> -	    status=$?
> -	    ;;
> -	meld)
> -	    touch "$BACKUP"
> -	    "$merge_tool_path" "$LOCAL" "$MERGED" "$REMOTE"
> -	    check_unchanged
> -	    ;;
> -	vimdiff)
> -	    touch "$BACKUP"
> -	    "$merge_tool_path" -c "wincmd l" "$LOCAL" "$MERGED" "$REMOTE"
> -	    check_unchanged
> -	    ;;
> -	gvimdiff)
> -	    touch "$BACKUP"
> -	    "$merge_tool_path" -c "wincmd l" -f "$LOCAL" "$MERGED" "$REMOTE"
> -	    check_unchanged
> -	    ;;
> -	xxdiff)
> -	    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 --show-merged-pane \
> -		    -R 'Accel.SaveAsMerged: "Ctrl-S"' \
> -		    -R 'Accel.Search: "Ctrl+F"' \
> -		    -R 'Accel.SearchForward: "Ctrl-G"' \
> -		    --merged-file "$MERGED" "$LOCAL" "$REMOTE"
> -	    fi
> -	    check_unchanged
> -	    ;;
> -	opendiff)
> -	    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
> -	    ;;
> -	ecmerge)
> -	    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
> -	    ;;
> -	emerge)
> -	    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=$?
> -	    ;;
> -	*)
> -	    if test -n "$merge_tool_cmd"; then
> -		if test "$merge_tool_trust_exit_code" = "false"; then
> -		    touch "$BACKUP"
> -		    ( eval $merge_tool_cmd )
> -		    check_unchanged
> -		else
> -		    ( eval $merge_tool_cmd )
> -		    status=$?
> -		fi
> -	    fi
> -	    ;;
> -    esac
> +    present=false
> +    base_present &&
> +    present=true
> +
> +    run_mergetool "$merge_tool" "$present"
> +    status=$?

This last line has to be deleted, because the variable 'status' set in
run_mergetool would be overwritten then. In this case the merge will
succeed even if it didn't and the file will be staged.

>      if test "$status" -ne 0; then
>  	echo "merge of $MERGED failed" 1>&2
>  	mv -- "$BACKUP" "$MERGED"

Re: [PATCH 03/10] Add a mergetool-lib scriptlet for holding common merge tool functions

[David Aguilar]

On  0, Markus Heidelberg <markus.heidelberg@web.de> wrote:
> David Aguilar, 01.04.2009:
> > 
> > diff --git a/git-mergetool-lib.sh b/git-mergetool-lib.sh
> > +valid_tool () {
> > +	case "$1" in
> > +	kdiff3 | kompare | tkdiff | xxdiff | meld | opendiff | emerge | vimdiff | gvimdiff | ecmerge)
> > +		if test "$1" = "kompare" && ! diff_mode; then
> > +			return 1
> > +		fi
> > +		;; # happy
> > +	*)
> > +		if ! test -n "$(get_custom_cmd "$1")"; then
> 
> Better this?
> 		if test -z "$(get_custom_cmd "$1")"; then
> 
> > +			return 1
> > +		fi ;;
> 
> For consistency:
> 		fi
> 		;;
> 


I'll reroll tonight and include you on the CC when I resent the
patches in question.

Thanks Markus.

-- 
		David

Re: git-{diff,merge} refactor round 2

[Charles Bailey]

On Wed, Apr 01, 2009 at 05:55:04AM -0700, David Aguilar wrote:
> Here's the 2nd round of refactoring.
 
Again, I reply in summary form...

[02/10] - I ack this.

[03-07 / 10] - fine. I'm not sure, but should they be one commit?
They're really about consolidating blocks from one place to another
and the change might be more 'obvious' as a single commit.

[08/10] Now that we've isolated the run_mergetool, can we get it to
return the value of '$status' (either the result of the tool or of
check_unchanged) as its exit code so that we know whether the merge
succeeded?

[10/10] Then this chunk:

+    run_mergetool "$merge_tool" "$present"
+    status=$?
     if test "$status" -ne 0; then

can validly become:

     if ! run_mergtool "$merge_tool" "$present"; then

which seems clearer to me.

-- 
Charles Bailey
http://ccgi.hashpling.plus.com/blog/

Re: [PATCH 10/10] mergetool: use run_mergetool from git-mergetool-lib

[Charles Bailey]

On Thu, Apr 02, 2009 at 12:54:22AM +0200, Markus Heidelberg wrote:
> David Aguilar, 01.04.2009:
> > +    present=false
> > +    base_present &&
> > +    present=true
> > +
> > +    run_mergetool "$merge_tool" "$present"
> > +    status=$?
> 
> This last line has to be deleted, because the variable 'status' set in
> run_mergetool would be overwritten then. In this case the merge will
> succeed even if it didn't and the file will be staged.

I think that it would be better if $status stayed as local as
possible. If run_mergetool returned the value of $status as its exit
code then the function will properly return whether the merge
succeeded or not.

This way run_mergetool's clients can just use its exit code, without
having to know about a magic $status global variable.

-- 
Charles Bailey
http://ccgi.hashpling.plus.com/blog/

Re: [PATCH 10/10] mergetool: use run_mergetool from git-mergetool-lib

[Markus Heidelberg]

Charles Bailey, 02.04.2009:
> On Thu, Apr 02, 2009 at 12:54:22AM +0200, Markus Heidelberg wrote:
> > David Aguilar, 01.04.2009:
> > > +    present=false
> > > +    base_present &&
> > > +    present=true
> > > +
> > > +    run_mergetool "$merge_tool" "$present"
> > > +    status=$?
> > 
> > This last line has to be deleted, because the variable 'status' set in
> > run_mergetool would be overwritten then. In this case the merge will
> > succeed even if it didn't and the file will be staged.
> 
> I think that it would be better if $status stayed as local as
> possible. If run_mergetool returned the value of $status as its exit
> code then the function will properly return whether the merge
> succeeded or not.
> 
> This way run_mergetool's clients can just use its exit code, without
> having to know about a magic $status global variable.

Sure. Next time I have to think about the right solution before
spreading workarounds.

And shouldn't also check_unchanged() move from git-mergetool.sh to
git-mergetool--lib.sh, since it is only used there?
Huh, it seems as if some editors for difftool cannot work correctly
currently, because this function is used, but defined in
git-mergetool.sh.

Markus

Re: [PATCH 10/10] mergetool: use run_mergetool from git-mergetool-lib

[Markus Heidelberg]

Markus Heidelberg, 02.04.2009:
> Huh, it seems as if some editors for difftool cannot work correctly
> currently, because this function is used, but defined in
> git-mergetool.sh.

Yes, it works...

# Overridden in git-mergetool
check_unchanged () {
	true
}

But that's not really nice, is it? Maybe we'd be better with a

    if merge_mode; then

inside this function and only define it in git-mergetool--lib.sh.

Markus

Re: [PATCH 10/10] mergetool: use run_mergetool from git-mergetool-lib

[David Aguilar]

On  0, Markus Heidelberg <markus.heidelberg@web.de> wrote:
> Charles Bailey, 02.04.2009:
> > 
> > I think that it would be better if $status stayed as local as
> > possible. If run_mergetool returned the value of $status as its exit
> > code then the function will properly return whether the merge
> > succeeded or not.
> > 
> > This way run_mergetool's clients can just use its exit code, without
> > having to know about a magic $status global variable.
> 
> And shouldn't also check_unchanged() move from git-mergetool.sh to
> git-mergetool--lib.sh, since it is only used there?
> Huh, it seems as if some editors for difftool cannot work correctly
> currently, because this function is used, but defined in
> git-mergetool.sh.

Sounds good.
A patch is on the way:

- moves check_unchanged into mergetool--lib
- returns $status in run_mergetool
- updates mergetool to not rely on the global $status

-- 
		David

Re: git-{diff,merge} refactor round 2

[Markus Heidelberg]

David Aguilar, 01.04.2009:
> Here's the 2nd round of refactoring.

I just noticed that mergetool.<mergetool>.path doesn't work anymore.
git grep mergetool.*path only hits one line in git-difftool--helper.sh
Neither does it seem to work with difftool, but I'm gonna go to bed now.

Markus

Re: git-{diff,merge} refactor round 2

[David Aguilar]

On  0, Markus Heidelberg <markus.heidelberg@web.de> wrote:
> David Aguilar, 01.04.2009:
> > Here's the 2nd round of refactoring.
> 
> I just noticed that mergetool.<mergetool>.path doesn't work anymore.
> git grep mergetool.*path only hits one line in git-difftool--helper.sh
> Neither does it seem to work with difftool, but I'm gonna go to bed now.
> 
> Markus
> 

Oops.  Well, I have one final patch that removes the last bit of
redundant code.  It also fixed this problem so I'll go ahead
and send it (it's based on top of da/difftool mentioned in
pu).

Since the test cases didn't catch that breakage I added a test
for it. 

Look for a patch called:

mergetool--lib: consolidate the last redundant bits in {diff,merge}tool


-- 

	David

Re: git-{diff,merge} refactor round 2

[Junio C Hamano]

David Aguilar <davvid@gmail.com> writes:

> On  0, Markus Heidelberg <markus.heidelberg@web.de> wrote:
>> David Aguilar, 01.04.2009:
>> > Here's the 2nd round of refactoring.
>> 
>> I just noticed that mergetool.<mergetool>.path doesn't work anymore.
>> git grep mergetool.*path only hits one line in git-difftool--helper.sh
>> Neither does it seem to work with difftool, but I'm gonna go to bed now.
>> 
>> Markus
>> 
>
> Oops.  Well, I have one final patch that removes the last bit of
> redundant code.  It also fixed this problem so I'll go ahead
> and send it (it's based on top of da/difftool mentioned in
> pu).
>
> Since the test cases didn't catch that breakage I added a test
> for it. 
>
> Look for a patch called:
>
> mergetool--lib: consolidate the last redundant bits in {diff,merge}tool

I'll try to queue all the outstanding da/difftool patches tonight, but I
think the patches in the series are getting to the point of needing a
fresh redoing.  Patches like "oops, these non-user scripts should have
been named with double-dash" can and should disappear.

Currently they are:

$ git log --oneline next..da/difftool
736e6b6 mergetool--lib: add new merge tool TortoiseMerge
b3ef7cc mergetool--lib: make (g)vimdiff workable under Windows
c4d690e mergetool--lib: consolidate the last redundant bits in {diff,merge}tool
def88c8 mergetool-lib: specialize opendiff options when in diff mode
bd52fab mergetool-lib: refactor run_mergetool and check_unchanged
e87266c bash completion: add git-difftool
04c3b54 {diff,merge}tool: rename helpers to remove them from tab-completion
2a83022 mergetool-lib: add diffuse as merge and diff tool
73c59d9 mergetool-lib: specialize xxdiff options when in diff mode
273e7a2 mergetool-lib: specialize kdiff3 options when in diff mode
99511d8 mergetool: use run_mergetool from git-mergetool-lib
37c48c7 difftool: use run_mergetool from git-mergetool-lib
4e314b5 mergetool-lib: introduce run_mergetool
588954e difftool: use valid_tool from git-mergetool-lib
8af4556 mergetool: use valid_tool from git-mergetool-lib
72286b5 difftool: use get_mergetool_path from git-mergetool-lib
d03b97f mergetool: use get_mergetool_path from git-mergetool-lib
c6afc72 Add a mergetool-lib scriptlet for holding common merge tool functions
6108b75 mergetool: use $( ... ) instead of `backticks`
73786e2 difftool: add support for a difftool.prompt config variable
472ff62 difftool: add a -y shortcut for --no-prompt
de2b85d difftool: use perl built-ins when testing for msys
9df990e difftool: add various git-difftool tests
8ac77f2 difftool: add git-difftool to the list of commands

Re: git-{diff,merge} refactor round 2

[David Aguilar]

On  0, Junio C Hamano <gitster@pobox.com> wrote:
> David Aguilar <davvid@gmail.com> writes:
> 
> I'll try to queue all the outstanding da/difftool patches tonight, but I
> think the patches in the series are getting to the point of needing a
> fresh redoing.  Patches like "oops, these non-user scripts should have
> been named with double-dash" can and should disappear.
> 
> Currently they are:
> 
> $ git log --oneline next..da/difftool
> 736e6b6 mergetool--lib: add new merge tool TortoiseMerge
> b3ef7cc mergetool--lib: make (g)vimdiff workable under Windows
> c4d690e mergetool--lib: consolidate the last redundant bits in {diff,merge}tool
> def88c8 mergetool-lib: specialize opendiff options when in diff mode
> bd52fab mergetool-lib: refactor run_mergetool and check_unchanged
> e87266c bash completion: add git-difftool
> 04c3b54 {diff,merge}tool: rename helpers to remove them from tab-completion
> 2a83022 mergetool-lib: add diffuse as merge and diff tool
> 73c59d9 mergetool-lib: specialize xxdiff options when in diff mode
> 273e7a2 mergetool-lib: specialize kdiff3 options when in diff mode
> 99511d8 mergetool: use run_mergetool from git-mergetool-lib
> 37c48c7 difftool: use run_mergetool from git-mergetool-lib
> 4e314b5 mergetool-lib: introduce run_mergetool
> 588954e difftool: use valid_tool from git-mergetool-lib
> 8af4556 mergetool: use valid_tool from git-mergetool-lib
> 72286b5 difftool: use get_mergetool_path from git-mergetool-lib
> d03b97f mergetool: use get_mergetool_path from git-mergetool-lib
> c6afc72 Add a mergetool-lib scriptlet for holding common merge tool functions
> 6108b75 mergetool: use $( ... ) instead of `backticks`
> 73786e2 difftool: add support for a difftool.prompt config variable
> 472ff62 difftool: add a -y shortcut for --no-prompt
> de2b85d difftool: use perl built-ins when testing for msys
> 9df990e difftool: add various git-difftool tests
> 8ac77f2 difftool: add git-difftool to the list of commands

It goes back even farther...

d3b8cec difftool: move 'git-difftool' out of contrib

d3db8cec is currently sitting in 'next' and is where the
"oops, I should have used double-dash" lack of foresight
began.

I like the *result* of what's currently sitting in
da/difftool, so rewriting history is easy now that we know
exactly where we're going.

I think we have a couple of options here.  I won't be able to
get to it until sometime tonight, but I'll throw my plan out
here to get a feel for what's the better way to do it.
I think we have a couple of options.  I'm open to any of
these:

1. Base it on the current master, completely throwing away
the existing da/difftool branch.  That would include throwing
away the commit that's in next if we really want to be clean
about the history.  In the process, move Markus' mergetool
fixes for windows to the top so that they can be applied
independently if necessary.  This series would then depend
on them.

This would probably mean a merge conflict with next at some
point.


2. Base the series on next, keeping the 'move out of contrib'
patch intact.  That'll minimize conflicts but will have an
extra commit that renames difftool-helper.  I can still push
the fixes-for-windows patches up to the top so that it makes
it easier on msysgit since we already have those two patches
in the msysgit 'tortoisemerge' branch.

3. Any others?


Regardless of which it's based on, it's obvious that there'll
be some squashing going on.  Tentatively,

Will be squashed:
588954e difftool: use valid_tool from git-mergetool-lib
8af4556 mergetool: use valid_tool from git-mergetool-lib

Will be squashed:
72286b5 difftool: use get_mergetool_path from git-mergetool-lib
d03b97f mergetool: use get_mergetool_path from git-mergetool-lib
c6afc72 Add a mergetool-lib scriptlet for holding common merge tool functions

Will be squashed:
99511d8 mergetool: use run_mergetool from git-mergetool-lib
37c48c7 difftool: use run_mergetool from git-mergetool-lib
4e314b5 mergetool-lib: introduce run_mergetool

Will be squashed:
def88c8 mergetool-lib: specialize opendiff options when in diff mode
73c59d9 mergetool-lib: specialize xxdiff options when in diff mode
273e7a2 mergetool-lib: specialize kdiff3 options when in diff mode


I'll go with whatever you guys think makes the series easiest to manage.

I think my preference would be to resend the entires series
based on 'master', but I don't want to make it any harder to
manage 'next'.

Thoughts?

-- 

	David

Re: git-{diff,merge} refactor round 2

[Markus Heidelberg]

David Aguilar, 05.04.2009:
> On  0, Junio C Hamano <gitster@pobox.com> wrote:
> > David Aguilar <davvid@gmail.com> writes:
> > 
> > I'll try to queue all the outstanding da/difftool patches tonight, but I
> > think the patches in the series are getting to the point of needing a
> > fresh redoing.  Patches like "oops, these non-user scripts should have
> > been named with double-dash" can and should disappear.
> > 
> > Currently they are:
> > 
> > $ git log --oneline next..da/difftool
> > [..]
> 
> It goes back even farther...
> 
> d3b8cec difftool: move 'git-difftool' out of contrib
> 
> d3db8cec is currently sitting in 'next' and is where the
> "oops, I should have used double-dash" lack of foresight
> began.

> 1. Base it on the current master, completely throwing away
> the existing da/difftool branch.  That would include throwing
> away the commit that's in next if we really want to be clean
> about the history.  In the process, move Markus' mergetool
> fixes for windows to the top so that they can be applied
> independently if necessary.  This series would then depend
> on them.

This is my favourite, too.

Additionally, what about basing these on master as well? They also are
unrelated to the refactoring:

def88c8 mergetool-lib: specialize opendiff options when in diff mode
2a83022 mergetool-lib: add diffuse as merge and diff tool
73c59d9 mergetool-lib: specialize xxdiff options when in diff mode
273e7a2 mergetool-lib: specialize kdiff3 options when in diff mode

And start refactoring on top of these?
Then these could go into master or next, since they are mostly bugfixes
and the refactoring can start in pu.

> This would probably mean a merge conflict with next at some
> point.

Revert "d3b8cec difftool: move 'git-difftool' out of contrib" from next?

> Regardless of which it's based on, it's obvious that there'll
> be some squashing going on.  Tentatively,
> 
> Will be squashed:
> 588954e difftool: use valid_tool from git-mergetool-lib
> 8af4556 mergetool: use valid_tool from git-mergetool-lib
> 
> Will be squashed:
> 72286b5 difftool: use get_mergetool_path from git-mergetool-lib
> d03b97f mergetool: use get_mergetool_path from git-mergetool-lib
> c6afc72 Add a mergetool-lib scriptlet for holding common merge tool functions
> 
> Will be squashed:
> 99511d8 mergetool: use run_mergetool from git-mergetool-lib
> 37c48c7 difftool: use run_mergetool from git-mergetool-lib
> 4e314b5 mergetool-lib: introduce run_mergetool
> 
> Will be squashed:
> def88c8 mergetool-lib: specialize opendiff options when in diff mode
> 73c59d9 mergetool-lib: specialize xxdiff options when in diff mode
> 273e7a2 mergetool-lib: specialize kdiff3 options when in diff mode

Yes, some squashing would be nice. Similar commit messages are confusing
when reading the history.

Markus

Re: git-{diff,merge} refactor round 2

[Junio C Hamano]

Markus Heidelberg <markus.heidelberg@web.de> writes:

> David Aguilar, 05.04.2009:
> ...
>> 1. Base it on the current master, completely throwing away
>> the existing da/difftool branch.  That would include throwing
>> away the commit that's in next if we really want to be clean
>> about the history.  In the process, move Markus' mergetool
>> fixes for windows to the top so that they can be applied
>> independently if necessary.  This series would then depend
>> on them.
>
> This is my favourite, too.

Ok.  You two forgot another obvious option of not doing anything, but
since both of you seem to be Ok with the rewrite, let's take that
approach.  It is easy to revert the one premature merge out of next and
then merge the cleaned-up version.

> Yes, some squashing would be nice. Similar commit messages are confusing
> when reading the history.

Agreed.