threads / discuss / 18681

git-{diff,merge} refactor round 2

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

## tl;dr

26 messages between Apr 1, 2009 and Apr 6, 2009.

replies: 25people: 4as markdown or json

David Aguilar· Apr 1, 2009, 12:55 UTC · lore
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.
David Aguilar· Apr 1, 2009, 12:55 UTC · re: David Aguilar · lore

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

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
David Aguilar· Apr 1, 2009, 12:55 UTC · re: David Aguilar · lore

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

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
David Aguilar· Apr 1, 2009, 12:55 UTC · re: David Aguilar · lore

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

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
David Aguilar· Apr 1, 2009, 12:55 UTC · re: David Aguilar · lore

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

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
David Aguilar· Apr 1, 2009, 12:55 UTC · re: David Aguilar · lore

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

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
David Aguilar· Apr 1, 2009, 12:55 UTC · re: David Aguilar · lore

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

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
David Aguilar· Apr 1, 2009, 12:55 UTC · re: David Aguilar · lore

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

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
David Aguilar· Apr 1, 2009, 12:55 UTC · re: David Aguilar · lore

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

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
David Aguilar· Apr 1, 2009, 12:55 UTC · re: David Aguilar · lore

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

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
David Aguilar· Apr 1, 2009, 12:55 UTC · re: David Aguilar · lore

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

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
Markus Heidelberg· Apr 1, 2009, 22:54 UTC · re: David Aguilar · lore

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

David Aguilar, 01.04.2009:
Show 111 quoted lines
> 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"
Charles Bailey· Apr 2, 2009, 20:02 UTC · re: Markus Heidelberg · lore

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

On Thu, Apr 02, 2009 at 12:54:22AM +0200, Markus Heidelberg wrote:
Show 11 quoted lines
> 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/
Markus Heidelberg· Apr 2, 2009, 20:13 UTC · re: Charles Bailey · lore

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

Charles Bailey, 02.04.2009:
Show 20 quoted lines
> 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
Markus Heidelberg· Apr 2, 2009, 20:16 UTC · re: Markus Heidelberg · lore

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

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
David Aguilar· Apr 3, 2009, 01:54 UTC · re: Markus Heidelberg · lore

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

On  0, Markus Heidelberg <markus.heidelberg@web.de> wrote:
Show 15 quoted lines
> 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
Markus Heidelberg· Apr 1, 2009, 22:47 UTC · re: David Aguilar · lore

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

David Aguilar, 01.04.2009:
Show 32 quoted lines
> 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.
Show 11 quoted lines
> +				"$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.
Show 88 quoted lines
> +		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
> +}
Markus Heidelberg· Apr 1, 2009, 22:39 UTC · re: David Aguilar · lore

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

David Aguilar, 01.04.2009:
Show 11 quoted lines
> 
> 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
		;;
David Aguilar· Apr 2, 2009, 03:58 UTC · re: Markus Heidelberg · lore

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

On  0, Markus Heidelberg <markus.heidelberg@web.de> wrote:
Show 23 quoted lines
> 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
Charles Bailey· Apr 2, 2009, 19:59 UTC · re: David Aguilar · lore

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

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/
Markus Heidelberg· Apr 5, 2009, 02:58 UTC · re: David Aguilar · lore

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

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
David Aguilar· Apr 5, 2009, 03:34 UTC · re: Markus Heidelberg · lore

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

On  0, Markus Heidelberg <markus.heidelberg@web.de> wrote:
Show 9 quoted lines
> 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
Junio C Hamano· Apr 5, 2009, 09:45 UTC · re: David Aguilar · lore

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

David Aguilar <davvid@gmail.com> writes:
Show 22 quoted lines
> 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

David Aguilar· Apr 5, 2009, 21:15 UTC · re: Junio C Hamano · lore

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

On  0, Junio C Hamano <gitster@pobox.com> wrote:
Show 34 quoted lines
> 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
Markus Heidelberg· Apr 5, 2009, 22:15 UTC · re: David Aguilar · lore

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

David Aguilar, 05.04.2009:
Show 20 quoted lines
> 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.
Show 7 quoted lines
> 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?
Show 21 quoted lines
> 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
Junio C Hamano· Apr 6, 2009, 00:33 UTC · re: Markus Heidelberg · lore

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

Markus Heidelberg <markus.heidelberg@web.de> writes:
Show 11 quoted lines
> 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.

← back to recent threads