[PATCH] mergetool--lib: Allow custom commands to override built-ins

[PATCH] mergetool--lib: Allow custom commands to override built-ins

[David Aguilar]

Allow users to override the default commands provided by the
mergetools/* scriptlets.

Users occasionally run into problems where they expect to be
able to override the built-in tool names.  The documentation
does not explicitly mention that built-ins cannot be overridden,
so it's easy to assume that it should work.

Lift this restriction so that built-in tools are handled the
same way as user-configured tools.  Add tests to guarantee this
behavior.

A nice benefit of this change is that it protects users from
having future versions of git trump their custom configuration
with a new built-in tool.

C.f.:

http://stackoverflow.com/questions/7435002/mergetool-from-gitconfig-being-ignored
http://thread.gmane.org/gmane.comp.version-control.msysgit/13188
http://thread.gmane.org/gmane.comp.version-control.git/148267

Signed-off-by: David Aguilar <davvid@gmail.com>
---
The Cc: list includes folks who were involved in the above threads.
Hopefully we can do without the workarounds now ;-)

 git-mergetool--lib.sh | 40 ++++++++++++++++++++++++++++++++++++++--
 mergetools/defaults   | 28 ++--------------------------
 t/t7610-mergetool.sh  | 13 +++++++++++++
 t/t7800-difftool.sh   | 11 +++++++++++
 4 files changed, 64 insertions(+), 28 deletions(-)

diff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh
index 54cb708..f013a03 100644
--- a/git-mergetool--lib.sh
+++ b/git-mergetool--lib.sh
@@ -104,13 +104,49 @@ run_merge_tool () {
 
 	if merge_mode
 	then
-		merge_cmd "$1"
+		run_merge_cmd "$1"
 	else
-		diff_cmd "$1"
+		run_diff_cmd "$1"
 	fi
 	return $status
 }
 
+# Run a either a configured or built-in diff tool
+run_diff_cmd () {
+	merge_tool_cmd="$(get_merge_tool_cmd "$1")"
+	if test -n "$merge_tool_cmd"
+	then
+		( eval $merge_tool_cmd )
+		status=$?
+		return $status
+	else
+		diff_cmd "$1"
+	fi
+}
+
+# Run a either a configured or built-in merge tool
+run_merge_cmd () {
+	merge_tool_cmd="$(get_merge_tool_cmd "$1")"
+	if test -n "$merge_tool_cmd"
+	then
+		trust_exit_code="$(git config --bool \
+			mergetool."$1".trustExitCode || echo false)"
+		if test "$trust_exit_code" = "false"
+		then
+			touch "$BACKUP"
+			( eval $merge_tool_cmd )
+			status=$?
+			check_unchanged
+		else
+			( eval $merge_tool_cmd )
+			status=$?
+		fi
+		return $status
+	else
+		merge_cmd "$1"
+	fi
+}
+
 list_merge_tool_candidates () {
 	if merge_mode
 	then
diff --git a/mergetools/defaults b/mergetools/defaults
index 1d8f2a3..21e63ec 100644
--- a/mergetools/defaults
+++ b/mergetools/defaults
@@ -8,36 +8,12 @@ can_diff () {
 }
 
 diff_cmd () {
-	merge_tool_cmd="$(get_merge_tool_cmd "$1")"
-	if test -z "$merge_tool_cmd"
-	then
-		status=1
-		break
-	fi
-	( eval $merge_tool_cmd )
-	status=$?
+	status=1
 	return $status
 }
 
 merge_cmd () {
-	merge_tool_cmd="$(get_merge_tool_cmd "$1")"
-	if test -z "$merge_tool_cmd"
-	then
-		status=1
-		break
-	fi
-	trust_exit_code="$(git config --bool \
-		mergetool."$1".trustExitCode || echo false)"
-	if test "$trust_exit_code" = "false"
-	then
-		touch "$BACKUP"
-		( eval $merge_tool_cmd )
-		status=$?
-		check_unchanged
-	else
-		( eval $merge_tool_cmd )
-		status=$?
-	fi
+	status=1
 	return $status
 }
 
diff --git a/t/t7610-mergetool.sh b/t/t7610-mergetool.sh
index 6fa0c70..bc38737 100755
--- a/t/t7610-mergetool.sh
+++ b/t/t7610-mergetool.sh
@@ -509,4 +509,17 @@ test_expect_success 'file with no base' '
     git reset --hard master >/dev/null 2>&1
 '
 
+test_expect_success 'custom commands override built-ins' '
+    git checkout -b test14 branch1 &&
+    git config mergetool.defaults.cmd "cat \"\$REMOTE\" >\"\$MERGED\"" &&
+    git config mergetool.defaults.trustExitCode true &&
+    test_must_fail git merge master &&
+    git mergetool --no-prompt --tool defaults -- both &&
+    echo master both added >expected &&
+    test_cmp both expected &&
+    git config --unset mergetool.defaults.cmd &&
+    git config --unset mergetool.defaults.trustExitCode &&
+    git reset --hard master >/dev/null 2>&1
+'
+
 test_done
diff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh
index 9c3e997..eb1d3f8 100755
--- a/t/t7800-difftool.sh
+++ b/t/t7800-difftool.sh
@@ -76,6 +76,17 @@ test_expect_success PERL 'custom commands' '
 	test "$diff" = "branch"
 '
 
+# Ensures that a custom difftool.<tool>.cmd overrides built-ins
+test_expect_success PERL 'custom commands override built-ins' '
+	restore_test_defaults &&
+	git config difftool.defaults.cmd "cat \$REMOTE" &&
+
+	diff=$(git difftool --tool defaults --no-prompt branch) &&
+	test "$diff" = "master" &&
+
+	git config --unset difftool.defaults.cmd
+'
+
 # Ensures that git-difftool ignores bogus --tool values
 test_expect_success PERL 'difftool ignores bad --tool values' '
 	diff=$(git difftool --no-prompt --tool=bad-tool branch)
-- 
1.7.12.1.402.g28d36ee

Re: [PATCH] mergetool--lib: Allow custom commands to override built-ins

[Ramkumar Ramachandra]

Hi David,

David Aguilar wrote:
>  diff_cmd () {
> -       merge_tool_cmd="$(get_merge_tool_cmd "$1")"
> -       if test -z "$merge_tool_cmd"
> -       then
> -               status=1
> -               break
> -       fi
> -       ( eval $merge_tool_cmd )
> -       status=$?
> +       status=1
>         return $status
>  }

Nit: Why not return 1, instead of setting $status and returning it?

Ram

Re: [PATCH] mergetool--lib: Allow custom commands to override built-ins

[Junio C Hamano]

Ramkumar Ramachandra <artagnon@gmail.com> writes:

> Hi David,
>
> David Aguilar wrote:
>>  diff_cmd () {
>> -       merge_tool_cmd="$(get_merge_tool_cmd "$1")"
>> -       if test -z "$merge_tool_cmd"
>> -       then
>> -               status=1
>> -               break
>> -       fi
>> -       ( eval $merge_tool_cmd )
>> -       status=$?
>> +       status=1
>>         return $status
>>  }
>
> Nit: Why not return 1, instead of setting $status and returning it?

Perhaps because the caller "run_merge_tool" pays attention to
$status that is a global variable?

Have you traced the call chain?

Re: [PATCH] mergetool--lib: Allow custom commands to override built-ins

[David Aguilar]

On Wed, Sep 26, 2012 at 7:06 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Ramkumar Ramachandra <artagnon@gmail.com> writes:
>
>> Hi David,
>>
>> David Aguilar wrote:
>>>  diff_cmd () {
>>> -       merge_tool_cmd="$(get_merge_tool_cmd "$1")"
>>> -       if test -z "$merge_tool_cmd"
>>> -       then
>>> -               status=1
>>> -               break
>>> -       fi
>>> -       ( eval $merge_tool_cmd )
>>> -       status=$?
>>> +       status=1
>>>         return $status
>>>  }
>>
>> Nit: Why not return 1, instead of setting $status and returning it?
>
> Perhaps because the caller "run_merge_tool" pays attention to
> $status that is a global variable?
>
> Have you traced the call chain?

Exactly.  I would like to eliminate globals whenever possible,
but this particular topic involved refactoring which aimed to keep
existing behavior w.r.t these variables unchanged.
-- 
David