[PATCH 1/2] Tidy up git mergetool's backup file behaviour and variable names

[PATCH 1/2] Tidy up git mergetool's backup file behaviour and variable names

[Charles Bailey]

Currently a backup pre-merge file with conflict markers is sometimes
kept with a .orig extenstion and sometimes removed depending on the
particular merge tool used.

This patch makes the handling consistent across all merge tools and
configurable via a new mergetool.keepBackup config variable

Changed the merge file path variable to MERGED for consistency with the
names of the merge temporary filename variables. This done with the
intention of having these variables used by user scripts in a subsequent
custom merge tool patch.

Signed-off-by: Charles Bailey <charles@hashpling.org>
---

Reordered patch series taking into account Junio's latest comments.


 Documentation/config.txt |    5 ++
 git-mergetool.sh         |  134 +++++++++++++++++++++-------------------------
 2 files changed, 67 insertions(+), 72 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index f9bdb16..c145d5b 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -717,6 +717,11 @@ mergetool.<tool>.path::
 	Override the path for the given tool.  This is useful in case
 	your tool is not in the PATH.
 
+mergetool.keepBackup::
+	After performing a merge, the original file with conflict markers
+	can be saved as a file with a `.orig` extension.  If this variable
+	is set to `false` then this file is not preserved.
+
 pack.window::
 	The size of the window used by linkgit:git-pack-objects[1] when no
 	window size is given on the command line. Defaults to 10.
diff --git a/git-mergetool.sh b/git-mergetool.sh
index cbbb707..f556927 100755
--- a/git-mergetool.sh
+++ b/git-mergetool.sh
@@ -34,7 +34,7 @@ base_present () {
 
 cleanup_temp_files () {
     if test "$1" = --save-backup ; then
-	mv -- "$BACKUP" "$path.orig"
+	mv -- "$BACKUP" "$MERGED.orig"
 	rm -f -- "$LOCAL" "$REMOTE" "$BASE"
     else
 	rm -f -- "$LOCAL" "$REMOTE" "$BASE" "$BACKUP"
@@ -67,14 +67,14 @@ resolve_symlink_merge () {
 	read ans
 	case "$ans" in
 	    [lL]*)
-		git checkout-index -f --stage=2 -- "$path"
-		git add -- "$path"
+		git checkout-index -f --stage=2 -- "$MERGED"
+		git add -- "$MERGED"
 		cleanup_temp_files --save-backup
 		return
 		;;
 	    [rR]*)
-		git checkout-index -f --stage=3 -- "$path"
-		git add -- "$path"
+		git checkout-index -f --stage=3 -- "$MERGED"
+		git add -- "$MERGED"
 		cleanup_temp_files --save-backup
 		return
 		;;
@@ -95,12 +95,12 @@ resolve_deleted_merge () {
 	read ans
 	case "$ans" in
 	    [mMcC]*)
-		git add -- "$path"
+		git add -- "$MERGED"
 		cleanup_temp_files --save-backup
 		return
 		;;
 	    [dD]*)
-		git rm -- "$path" > /dev/null
+		git rm -- "$MERGED" > /dev/null
 		cleanup_temp_files
 		return
 		;;
@@ -112,11 +112,11 @@ resolve_deleted_merge () {
 }
 
 check_unchanged () {
-    if test "$path" -nt "$BACKUP" ; then
+    if test "$MERGED" -nt "$BACKUP" ; then
 	status=0;
     else
 	while true; do
-	    echo "$path seems unchanged."
+	    echo "$MERGED seems unchanged."
 	    printf "Was the merge successful? [y/n] "
 	    read answer < /dev/tty
 	    case "$answer" in
@@ -127,50 +127,38 @@ check_unchanged () {
     fi
 }
 
-save_backup () {
-    if test "$status" -eq 0; then
-	mv -- "$BACKUP" "$path.orig"
-    fi
-}
-
-remove_backup () {
-    if test "$status" -eq 0; then
-	rm "$BACKUP"
-    fi
-}
-
 merge_file () {
-    path="$1"
+    MERGED="$1"
 
-    f=`git ls-files -u -- "$path"`
+    f=`git ls-files -u -- "$MERGED"`
     if test -z "$f" ; then
-	if test ! -f "$path" ; then
-	    echo "$path: file not found"
+	if test ! -f "$MERGED" ; then
+	    echo "$MERGED: file not found"
 	else
-	    echo "$path: file does not need merging"
+	    echo "$MERGED: file does not need merging"
 	fi
 	exit 1
     fi
 
-    ext="$$$(expr "$path" : '.*\(\.[^/]*\)$')"
-    BACKUP="$path.BACKUP.$ext"
-    LOCAL="$path.LOCAL.$ext"
-    REMOTE="$path.REMOTE.$ext"
-    BASE="$path.BASE.$ext"
+    ext="$$$(expr "$MERGED" : '.*\(\.[^/]*\)$')"
+    BACKUP="$MERGED.BACKUP.$ext"
+    LOCAL="$MERGED.LOCAL.$ext"
+    REMOTE="$MERGED.REMOTE.$ext"
+    BASE="$MERGED.BASE.$ext"
 
-    mv -- "$path" "$BACKUP"
-    cp -- "$BACKUP" "$path"
+    mv -- "$MERGED" "$BACKUP"
+    cp -- "$BACKUP" "$MERGED"
 
-    base_mode=`git ls-files -u -- "$path" | awk '{if ($3==1) print $1;}'`
-    local_mode=`git ls-files -u -- "$path" | awk '{if ($3==2) print $1;}'`
-    remote_mode=`git ls-files -u -- "$path" | 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   && git cat-file blob ":1:$prefix$path" >"$BASE" 2>/dev/null
-    local_present  && git cat-file blob ":2:$prefix$path" >"$LOCAL" 2>/dev/null
-    remote_present && git cat-file blob ":3:$prefix$path" >"$REMOTE" 2>/dev/null
+    base_present   && git cat-file blob ":1:$prefix$MERGED" >"$BASE" 2>/dev/null
+    local_present  && git cat-file blob ":2:$prefix$MERGED" >"$LOCAL" 2>/dev/null
+    remote_present && git cat-file blob ":3:$prefix$MERGED" >"$REMOTE" 2>/dev/null
 
     if test -z "$local_mode" -o -z "$remote_mode"; then
-	echo "Deleted merge conflict for '$path':"
+	echo "Deleted merge conflict for '$MERGED':"
 	describe_file "$local_mode" "local" "$LOCAL"
 	describe_file "$remote_mode" "remote" "$REMOTE"
 	resolve_deleted_merge
@@ -178,14 +166,14 @@ merge_file () {
     fi
 
     if is_symlink "$local_mode" || is_symlink "$remote_mode"; then
-	echo "Symbolic link merge conflict for '$path':"
+	echo "Symbolic link merge conflict for '$MERGED':"
 	describe_file "$local_mode" "local" "$LOCAL"
 	describe_file "$remote_mode" "remote" "$REMOTE"
 	resolve_symlink_merge
 	return
     fi
 
-    echo "Normal merge conflict for '$path':"
+    echo "Normal merge conflict for '$MERGED':"
     describe_file "$local_mode" "local" "$LOCAL"
     describe_file "$remote_mode" "remote" "$REMOTE"
     printf "Hit return to start merge resolution tool (%s): " "$merge_tool"
@@ -194,36 +182,32 @@ merge_file () {
     case "$merge_tool" in
 	kdiff3)
 	    if base_present ; then
-		("$merge_tool_path" --auto --L1 "$path (Base)" --L2 "$path (Local)" --L3 "$path (Remote)" \
-		    -o "$path" -- "$BASE" "$LOCAL" "$REMOTE" > /dev/null 2>&1)
+		("$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 "$path (Local)" --L2 "$path (Remote)" \
-		    -o "$path" -- "$LOCAL" "$REMOTE" > /dev/null 2>&1)
+		("$merge_tool_path" --auto --L1 "$MERGED (Local)" --L2 "$MERGED (Remote)" \
+		    -o "$MERGED" -- "$LOCAL" "$REMOTE" > /dev/null 2>&1)
 	    fi
 	    status=$?
-	    remove_backup
 	    ;;
 	tkdiff)
 	    if base_present ; then
-		"$merge_tool_path" -a "$BASE" -o "$path" -- "$LOCAL" "$REMOTE"
+		"$merge_tool_path" -a "$BASE" -o "$MERGED" -- "$LOCAL" "$REMOTE"
 	    else
-		"$merge_tool_path" -o "$path" -- "$LOCAL" "$REMOTE"
+		"$merge_tool_path" -o "$MERGED" -- "$LOCAL" "$REMOTE"
 	    fi
 	    status=$?
-	    save_backup
 	    ;;
 	meld|vimdiff)
 	    touch "$BACKUP"
-	    "$merge_tool_path" -- "$LOCAL" "$path" "$REMOTE"
+	    "$merge_tool_path" -- "$LOCAL" "$MERGED" "$REMOTE"
 	    check_unchanged
-	    save_backup
 	    ;;
 	gvimdiff)
-		touch "$BACKUP"
-		"$merge_tool_path" -f -- "$LOCAL" "$path" "$REMOTE"
-		check_unchanged
-		save_backup
-		;;
+	    touch "$BACKUP"
+	    "$merge_tool_path" -f -- "$LOCAL" "$MERGED" "$REMOTE"
+	    check_unchanged
+	    ;;
 	xxdiff)
 	    touch "$BACKUP"
 	    if base_present ; then
@@ -231,53 +215,57 @@ merge_file () {
 		    -R 'Accel.SaveAsMerged: "Ctrl-S"' \
 		    -R 'Accel.Search: "Ctrl+F"' \
 		    -R 'Accel.SearchForward: "Ctrl-G"' \
-		    --merged-file "$path" -- "$LOCAL" "$BASE" "$REMOTE"
+		    --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 "$path" -- "$LOCAL" "$REMOTE"
+		    --merged-file "$MERGED" -- "$LOCAL" "$REMOTE"
 	    fi
 	    check_unchanged
-	    save_backup
 	    ;;
 	opendiff)
 	    touch "$BACKUP"
 	    if base_present; then
-		"$merge_tool_path" "$LOCAL" "$REMOTE" -ancestor "$BASE" -merge "$path" | cat
+		"$merge_tool_path" "$LOCAL" "$REMOTE" -ancestor "$BASE" -merge "$MERGED" | cat
 	    else
-		"$merge_tool_path" "$LOCAL" "$REMOTE" -merge "$path" | cat
+		"$merge_tool_path" "$LOCAL" "$REMOTE" -merge "$MERGED" | cat
 	    fi
 	    check_unchanged
-	    save_backup
 	    ;;
 	ecmerge)
 	    touch "$BACKUP"
 	    if base_present; then
-		"$merge_tool_path" "$BASE" "$LOCAL" "$REMOTE" --mode=merge3 --to="$path"
+		"$merge_tool_path" "$BASE" "$LOCAL" "$REMOTE" --mode=merge3 --to="$MERGED"
 	    else
-		"$merge_tool_path" "$LOCAL" "$REMOTE" --mode=merge2 --to="$path"
+		"$merge_tool_path" "$LOCAL" "$REMOTE" --mode=merge2 --to="$MERGED"
 	    fi
 	    check_unchanged
-	    save_backup
 	    ;;
 	emerge)
 	    if base_present ; then
-		"$merge_tool_path" -f emerge-files-with-ancestor-command "$LOCAL" "$REMOTE" "$BASE" "$(basename "$path")"
+		"$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 "$path")"
+		"$merge_tool_path" -f emerge-files-command "$LOCAL" "$REMOTE" "$(basename "$MERGED")"
 	    fi
 	    status=$?
-	    save_backup
 	    ;;
     esac
+
     if test "$status" -ne 0; then
-	echo "merge of $path failed" 1>&2
-	mv -- "$BACKUP" "$path"
+	echo "merge of $MERGED failed" 1>&2
+	mv -- "$BACKUP" "$MERGED"
 	exit 1
     fi
-    git add -- "$path"
+
+    if test "$merge_keep_backup" = "true"; then
+	mv -- "$BACKUP" "$MERGED.orig"
+    else
+	rm -- "$BACKUP"
+    fi
+
+    git add -- "$MERGED"
     cleanup_temp_files
 }
 
@@ -380,6 +368,8 @@ else
 
     init_merge_tool_path "$merge_tool"
 
+    merge_keep_backup="$(git config --bool merge.keepBackup || echo true)"
+
     if ! type "$merge_tool_path" > /dev/null 2>&1; then
         echo "The merge tool $merge_tool is not available as '$merge_tool_path'"
         exit 1
-- 
1.5.4.1.34.g94bf

[PATCH 2/2] Teach git mergetool to use custom commands defined at config time

[Charles Bailey]

Currently git mergetool is restricted to a set of commands defined
in the script. You can subvert the mergetool.<tool>.path to force
git mergetool to use a different command, but if you have a command
whose invocation syntax does not match one of the current tools then
you would have to write a wrapper script for it.

This patch adds three git config variable patterns which allow a more
flexible choice of merge tool.

If you run git mergetool with -t/--tool or the merge.tool config
variable set to an unrecognized tool then git mergetool will query the
mergetool.<tool>.cmd config variable. If this variable exists, then
git mergetool will treat the specified tool as a custom command and
will use a shell eval to run the command so that the appropriate shell
variables can be used to find the merge temporary files.

mergetool.<tool>.trustExitCode can be used to indicate that the exit
code of the custom command can be used to determine the success of the
merge. mergetool.<tool>.keepBackup can be used to specify whether the
original pre-merge file with conflict markers should be kept as a
'.orig' file after the merge tool completes.

Signed-off-by: Charles Bailey <charles@hashpling.org>
---
 Documentation/config.txt |   25 +++++++++++++++++++++++--
 git-mergetool.sh         |   29 +++++++++++++++++++++++++++--
 2 files changed, 50 insertions(+), 4 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index c145d5b..60aa0d7 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -689,8 +689,10 @@ merge.summary::
 
 merge.tool::
 	Controls which merge resolution program is used by
-	linkgit:git-mergetool[1].  Valid values are: "kdiff3", "tkdiff",
-	"meld", "xxdiff", "emerge", "vimdiff", "gvimdiff", and "opendiff".
+	linkgit:git-mergetool[1].  Valid built-in values are: "kdiff3",
+	"tkdiff", "meld", "xxdiff", "emerge", "vimdiff", "gvimdiff", and
+	"opendiff".  Any other value is treated is custom merge tool
+	and there must be a corresponing mergetool.<tool>.cmd option.
 
 merge.verbosity::
 	Controls the amount of output shown by the recursive merge
@@ -717,6 +719,25 @@ mergetool.<tool>.path::
 	Override the path for the given tool.  This is useful in case
 	your tool is not in the PATH.
 
+mergetool.<tool>.cmd::
+	Specify the command to invoke the specified merge tool.  The
+	specified command is evaluated in shell with the following
+	variables available: 'BASE' is the name of a temporary file
+	containing the common base of the files to be merged, if available;
+	'LOCAL' is the name of a temporary file containing the contents of
+	the file on the current branch; 'REMOTE' is the name of a temporary
+	file containing the contents of the file from the branch being
+	merged; 'MERGED' contains the name of the file to which the merge
+	tool should write the results of a successful merge.
+
+mergetool.<tool>.trustExitCode::
+	For a custom merge command, specify whether the exit code of
+	the merge command can be used to determine whether the merge was
+	successful.  If this is not set to true then the merge target file
+	timestamp is checked and the merge assumed to have been successful
+	if the file has been updated, otherwise the user is prompted to
+	indicate the success of the merge.
+
 mergetool.keepBackup::
 	After performing a merge, the original file with conflict markers
 	can be saved as a file with a `.orig` extension.  If this variable
diff --git a/git-mergetool.sh b/git-mergetool.sh
index f556927..fd0812b 100755
--- a/git-mergetool.sh
+++ b/git-mergetool.sh
@@ -251,6 +251,19 @@ merge_file () {
 	    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
+		save_backup
+	    fi
+	    ;;
     esac
 
     if test "$status" -ne 0; then
@@ -297,12 +310,20 @@ 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
 		*)
-			return 1
+			if ! valid_custom_tool "$1"; then
+				return 1
+			fi
 			;;
 	esac
 }
@@ -370,10 +391,14 @@ else
 
     merge_keep_backup="$(git config --bool merge.keepBackup || echo true)"
 
-    if ! type "$merge_tool_path" > /dev/null 2>&1; then
+    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'"
         exit 1
     fi
+
+    if ! test -z "$merge_tool_cmd"; then
+        merge_tool_trust_exit_code="$(git config --bool mergetool.$merge_tool.trustExitCode || echo false)"
+    fi
 fi
 
 
-- 
1.5.4.1.34.g94bf


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

Re: [PATCH 1/2] Tidy up git mergetool's backup file behaviour and variable names

[Junio C Hamano]

Charles Bailey <charles@hashpling.org> writes:

> Currently a backup pre-merge file with conflict markers is sometimes
> kept with a .orig extenstion and sometimes removed depending on the
> particular merge tool used.
>
> This patch makes the handling consistent across all merge tools and
> configurable via a new mergetool.keepBackup config variable
>
> Changed the merge file path variable to MERGED for consistency with the
> names of the merge temporary filename variables. This done with the
> intention of having these variables used by user scripts in a subsequent
> custom merge tool patch.

I would have preferred two separate patches, one s/path/MERGED/
and the other save/remove clean-up, which would be much easier
way to review, but this is what we have, so let's work on this
version.

> +mergetool.keepBackup::
> +	After performing a merge, the original file with conflict markers
> +	can be saved as a file with a `.orig` extension.  If this variable
> +	is set to `false` then this file is not preserved.
> +

s/$/  Defaults to true (i.e. keep the backup files)/.

We might also want a command line option to override the user's
usual default specified with this configuration but that can be
left for later rounds.

> @@ -112,11 +112,11 @@ resolve_deleted_merge () {
>  }
>  
>  check_unchanged () {
> -    if test "$path" -nt "$BACKUP" ; then
> +    if test "$MERGED" -nt "$BACKUP" ; then

I think this is the cause of your automated test sometimes
needing 1 sec sleep.  This check should perhaps be done with a
"!  cmp $MERGED $BACKUP >/dev/null", which would succeed if the
user's interaction with the backend tool touched the file.

The rest of the patch looked fine to me.

We might also want to add -y (assume "Yes" answer to "Did you
resolve it" question without actually asking) command line
option to the script, but that would be for later rounds.

Re: [PATCH 1/2] Tidy up git mergetool's backup file behaviour and variable names

[Charles Bailey]

On Sun, Feb 17, 2008 at 07:45:42PM -0800, Junio C Hamano wrote:
> Charles Bailey <charles@hashpling.org> writes:
> 
> > Currently a backup pre-merge file with conflict markers is sometimes
> > kept with a .orig extenstion and sometimes removed depending on the
> > particular merge tool used.
> >
> > This patch makes the handling consistent across all merge tools and
> > configurable via a new mergetool.keepBackup config variable
> >
> > Changed the merge file path variable to MERGED for consistency with the
> > names of the merge temporary filename variables. This done with the
> > intention of having these variables used by user scripts in a subsequent
> > custom merge tool patch.
> 
> I would have preferred two separate patches, one s/path/MERGED/
> and the other save/remove clean-up, which would be much easier
> way to review, but this is what we have, so let's work on this
> version.

Fair enough, I'll send an update.
> 
> > +mergetool.keepBackup::
> > +	After performing a merge, the original file with conflict markers
> > +	can be saved as a file with a `.orig` extension.  If this variable
> > +	is set to `false` then this file is not preserved.
> > +
> 
> s/$/  Defaults to true (i.e. keep the backup files)/.

Noted.

> We might also want a command line option to override the user's
> usual default specified with this configuration but that can be
> left for later rounds.

Agreed, I'll tackle this later, given time.

> > @@ -112,11 +112,11 @@ resolve_deleted_merge () {
> >  }
> >  
> >  check_unchanged () {
> > -    if test "$path" -nt "$BACKUP" ; then
> > +    if test "$MERGED" -nt "$BACKUP" ; then
> 
> I think this is the cause of your automated test sometimes
> needing 1 sec sleep.  This check should perhaps be done with a
> "!  cmp $MERGED $BACKUP >/dev/null", which would succeed if the
> user's interaction with the backend tool touched the file.

I don't think that we should spend too much time on this seeing as I
now fail to reproduce it, but as the code was avoiding the interactive
path (i.e. trusting the exit code), it shouldn't have been this check.

> 
> The rest of the patch looked fine to me.
> 
> We might also want to add -y (assume "Yes" answer to "Did you
> resolve it" question without actually asking) command line
> option to the script, but that would be for later rounds.

Yes, and this would help the automated tests.