[PATCH] mergetools: add support for DeltaWalker

[PATCH] mergetools: add support for DeltaWalker

[Tim Henigan]

Since bc7a96a8965d, integration with external diff/merge tools requires
a mergetools/$tool file to be present.

This commits adds support for DeltaWalker.

Signed-off-by: Tim Henigan <tim.henigan@gmail.com>
---

Tested with DeltaWalker v1.9.8 on Ubuntu 11.10 and msysgit on Win7.


 mergetools/DeltaWalker |   13 +++++++++++++
 1 file changed, 13 insertions(+)
 create mode 100644 mergetools/DeltaWalker

diff --git a/mergetools/DeltaWalker b/mergetools/DeltaWalker
new file mode 100644
index 0000000..ae66686
--- /dev/null
+++ b/mergetools/DeltaWalker
@@ -0,0 +1,13 @@
+diff_cmd () {
+       "$merge_tool_path" "$LOCAL" "$REMOTE" >/dev/null 2>&1
+}
+
+merge_cmd () {
+       if $base_present
+       then
+               "$merge_tool_path" "$LOCAL" "$REMOTE" "$BASE" -merged="$PWD/$MERGED" >/dev/null 2>&1
+       else
+               "$merge_tool_path" "$LOCAL" "$REMOTE" -merged="$PWD/$MERGED" >/dev/null 2>&1
+       fi
+       status=$?
+}
-- 
1.7.9.GIT

Re: [PATCH] mergetools: add support for DeltaWalker

[Junio C Hamano]

Tim Henigan <tim.henigan@gmail.com> writes:

> Since bc7a96a8965d, integration with external diff/merge tools requires
> a mergetools/$tool file to be present.
>
> This commits adds support for DeltaWalker.
>
> Signed-off-by: Tim Henigan <tim.henigan@gmail.com>
> ---
>
> Tested with DeltaWalker v1.9.8 on Ubuntu 11.10 and msysgit on Win7.
>
>
>  mergetools/DeltaWalker |   13 +++++++++++++
>  1 file changed, 13 insertions(+)
>  create mode 100644 mergetools/DeltaWalker

I see that the earlier refactoring to make mergetool backend pluggable is
starting to pay off rather nicely.  It is not "since ..., requires ...",
but "thanks to ..., adding a random new tool is just a matter of dropping
a trivial shell snippet in the directory".

> diff --git a/mergetools/DeltaWalker b/mergetools/DeltaWalker
> new file mode 100644
> index 0000000..ae66686
> --- /dev/null
> +++ b/mergetools/DeltaWalker
> @@ -0,0 +1,13 @@
> +diff_cmd () {
> +       "$merge_tool_path" "$LOCAL" "$REMOTE" >/dev/null 2>&1
> +}

I noticed that most if not all of the mergetool plug-ins have these ugly
/dev/null redirects everywhere, and I also suspect that this may be
inherited from the first bad instance with copy-and-paste.  I was about to
suggest doing these redirection on the calling side in git-mergetool.sh
but there are some that do not want them, so perhaps each plug-in needs to
decide if they want redirection individually.

> +merge_cmd () {
> +       if $base_present
> +       then
> +               "$merge_tool_path" "$LOCAL" "$REMOTE" "$BASE" -merged="$PWD/$MERGED" >/dev/null 2>&1
> +       else
> +               "$merge_tool_path" "$LOCAL" "$REMOTE" -merged="$PWD/$MERGED" >/dev/null 2>&1
> +       fi

Perhaps doing the above like this might make it a bit less of an eye-sore.

	if $base_present
        then
		"$merge_tool_path" "$LOCAL" "$REMOTE" "$BASE" -merged="$PWD/$MERGED"
	else
		"$merge_tool_path" "$LOCAL" "$REMOTE" -merged="$PWD/$MERGED"
	fi >/dev/null 2>&1

> +       status=$?

This is highly dubious.  Looking at existing mergetools/*, I think the
caller expects merge_cmd to signal success or failure with $?, so you
probably just want to drop this line; the caller will then get the $?
that was set by the "$merge_tool_path" command.

That is how your diff_cmd is communicating with its caller after all, no?

> +}

Re: [PATCH] mergetools: add support for DeltaWalker

[Tim Henigan]

On Thu, Mar 1, 2012 at 11:37 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Tim Henigan <tim.henigan@gmail.com> writes:
>
> I see that the earlier refactoring to make mergetool backend pluggable is
> starting to pay off rather nicely.  It is not "since ..., requires ...",
> but "thanks to ..., adding a random new tool is just a matter of dropping
> a trivial shell snippet in the directory".

I will reword the commit message in v2.


> Perhaps doing the above like this might make it a bit less of an eye-sore.
>
>        if $base_present
>        then
>                "$merge_tool_path" "$LOCAL" "$REMOTE" "$BASE" -merged="$PWD/$MERGED"
>        else
>                "$merge_tool_path" "$LOCAL" "$REMOTE" -merged="$PWD/$MERGED"
>        fi >/dev/null 2>&1

Will update in v2.


>> +       status=$?
>
> This is highly dubious.  Looking at existing mergetools/*, I think the
> caller expects merge_cmd to signal success or failure with $?, so you
> probably just want to drop this line; the caller will then get the $?
> that was set by the "$merge_tool_path" command.
>
> That is how your diff_cmd is communicating with its caller after all, no?

It seems that only two of the existing scripts use this 'status=$?'
line (emerge and kdiff3).  I used the 'kdiff3' file as my starting
point without actually trying to understand why that line was present.
 Local testing shows that I don't need this line for DeltaWalker, so
it will be removed in v2.