[PATCH v2] mergetools: add support for DeltaWalker

[PATCH v2] mergetools: add support for DeltaWalker

[Tim Henigan]

Thanks to bc7a96a8965d, it is possible to integrate new external
diff/merge tools by adding a simple shell script at mergetools/$tool.

This commit adds DeltaWalker support.

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

Changes in v2:
  - reworded the commit message
  - moved >/dev/null redirect to after the final fi statement
  - removed the 'status=$?' line at the end of merge_cmd()

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


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

diff --git a/mergetools/DeltaWalker b/mergetools/DeltaWalker
new file mode 100644
index 0000000..b9e6618
--- /dev/null
+++ b/mergetools/DeltaWalker
@@ -0,0 +1,12 @@
+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"
+	else
+		"$merge_tool_path" "$LOCAL" "$REMOTE" -merged="$PWD/$MERGED"
+	fi >/dev/null 2>&1
+}
-- 
1.7.9.2.315.g25a78

Re: [PATCH v2] mergetools: add support for DeltaWalker

[Junio C Hamano]

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

>  mergetools/DeltaWalker |   12 ++++++++++++

How does an end user choose to use this backend?  Perhaps like this?

    $ git mergetool --tool=DeltaWalker

All the other files in mergetools/ are in lower case, and I _strongly_
prefer to have this new file also be in lower case.

Such a change may mean you may have to override translate_merge_tool_path
in this file, like some other backends seem to do.

>  1 file changed, 12 insertions(+)
>  create mode 100644 mergetools/DeltaWalker
>
> diff --git a/mergetools/DeltaWalker b/mergetools/DeltaWalker
> new file mode 100644
> index 0000000..b9e6618
> --- /dev/null
> +++ b/mergetools/DeltaWalker
> @@ -0,0 +1,12 @@
> +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"
> +	else
> +		"$merge_tool_path" "$LOCAL" "$REMOTE" -merged="$PWD/$MERGED"
> +	fi >/dev/null 2>&1
> +}

Re: [PATCH v2] mergetools: add support for DeltaWalker

[David Aguilar]

On Fri, Mar 2, 2012 at 2:39 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Tim Henigan <tim.henigan@gmail.com> writes:
>
>>  mergetools/DeltaWalker |   12 ++++++++++++
>
> How does an end user choose to use this backend?  Perhaps like this?
>
>    $ git mergetool --tool=DeltaWalker
>
> All the other files in mergetools/ are in lower case, and I _strongly_
> prefer to have this new file also be in lower case.

I agree.

> Such a change may mean you may have to override translate_merge_tool_path
> in this file, like some other backends seem to do.
>
>>  1 file changed, 12 insertions(+)
>>  create mode 100644 mergetools/DeltaWalker
>>
>> diff --git a/mergetools/DeltaWalker b/mergetools/DeltaWalker
>> new file mode 100644
>> index 0000000..b9e6618
>> --- /dev/null
>> +++ b/mergetools/DeltaWalker
>> @@ -0,0 +1,12 @@
>> +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"
>> +     else
>> +             "$merge_tool_path" "$LOCAL" "$REMOTE" -merged="$PWD/$MERGED"
>> +     fi >/dev/null 2>&1
>> +}

Is the $PWD/ prefix strictly needed?  The rest of the mergetools use
$MERGED as-is.  Does it work without it?
-- 
David

Re: [PATCH v2] mergetools: add support for DeltaWalker

[Junio C Hamano]

David Aguilar <davvid@gmail.com> writes:

>>> +merge_cmd () {
>>> +     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
>>> +}
>
> Is the $PWD/ prefix strictly needed?  The rest of the mergetools use
> $MERGED as-is.  Does it work without it?

Hrm, I didn't notice it but they do look fishy.  Thanks for good eyes.

Tim?

Re: [PATCH v2] mergetools: add support for DeltaWalker

[Tim Henigan]

On Sat, Mar 3, 2012 at 5:47 PM, Junio C Hamano <gitster@pobox.com> wrote:
> David Aguilar <davvid@gmail.com> writes:
>
>>>> +merge_cmd () {
>>>> +     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
>>>> +}
>>
>> Is the $PWD/ prefix strictly needed?  The rest of the mergetools use
>> $MERGED as-is.  Does it work without it?
>
> Hrm, I didn't notice it but they do look fishy.  Thanks for good eyes.
>
> Tim?

I ran a quick test using msysgit v1.7.9 on Win7 64-bit and found that
it fails without '$PWD'.

When '$PWD/' is removed from the '-merged' option, it results in a
Java JRE crash and the conflict resolutions entered by the user are
not written to the file.

The JRE exception is 'EXCEPTION_ACCESS_VIOLATION (0xc0000005)'.  I
posted the full text of the exception to a public location, in case
anyone is interested [1].

The format of the '-merged' option was copied directly from the
DeltaWalker manual.  There was no explanation why '$PWD' is needed.

[1]: https://gist.github.com/1970590

Re: [PATCH v2] mergetools: add support for DeltaWalker

[David Aguilar]

On Sat, Mar 3, 2012 at 7:50 PM, Tim Henigan <tim.henigan@gmail.com> wrote:
> On Sat, Mar 3, 2012 at 5:47 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> David Aguilar <davvid@gmail.com> writes:
>>
>>>>> +merge_cmd () {
>>>>> +     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
>>>>> +}
>>>
>>> Is the $PWD/ prefix strictly needed?  The rest of the mergetools use
>>> $MERGED as-is.  Does it work without it?
>>
>> Hrm, I didn't notice it but they do look fishy.  Thanks for good eyes.
>>
>> Tim?
>
> I ran a quick test using msysgit v1.7.9 on Win7 64-bit and found that
> it fails without '$PWD'.
>
> When '$PWD/' is removed from the '-merged' option, it results in a
> Java JRE crash and the conflict resolutions entered by the user are
> not written to the file.
>
> The JRE exception is 'EXCEPTION_ACCESS_VIOLATION (0xc0000005)'.  I
> posted the full text of the exception to a public location, in case
> anyone is interested [1].
>
> The format of the '-merged' option was copied directly from the
> DeltaWalker manual.  There was no explanation why '$PWD' is needed.
>
> [1]: https://gist.github.com/1970590

Thanks, that makes a lot of sense.  It looks like they have a bug.  Do
they know about it?

My naive guess is that they are using some inotify-like thing and
subscribing to a directory (per the stack trace).  Since
dirname("relative") is "" they crash.

So this bug probably wouldn't be present when merging a file in a
sub-directory, or ../ relative path.  Is this the case?

Can you mention this in a comment so that someone doesn't copy/paste
it into another tool in the future?

If they fix it then we may want to consider only supporting the newer
version and remove the $PWD.  Does the OS X version have this bug too?

I didn't notice any other production shell scripts in git using $PWD.
There is one use of `pwd` in git-stash.sh and it's used in a few
tests, as is $PWD, but $(pwd) is the most prevalent overall.  I don't
know the reason $PWD is avoided in the git shell scripts (though
portability is often the reason).

Deltawalker is not currently available on any of the platforms where
this would be a concern, but we may still want to use $(pwd) for
consistency with the other commands.  Does that work instead?
-- 
David

Re: [PATCH v2] mergetools: add support for DeltaWalker

[Junio C Hamano]

David Aguilar <davvid@gmail.com> writes:

> Can you mention this in a comment so that someone doesn't copy/paste
> it into another tool in the future?

Ok, I think the result should look like this, but could you fill in the
details in the below?

-- >8 --
From: Tim Henigan <tim.henigan@gmail.com>
Date: Sat, 3 Mar 2012 11:56:34 -0500
Subject: [PATCH] mergetools: add a plug-in to support DeltaWalker

DeltaWalker is a non-free tool that is popular among xxxxxxx users. Add a
plug-in to support it from difftool and mergetool.

Note that $(pwd)/ in front of $MERGED shouldn't be necessary, but without
it, DeltaWalker crashes with an JRE exception.

Signed-off-by: Tim Henigan <tim.henigan@gmail.com>
Helped-by: David Aguilar
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 mergetools/deltawalker |   19 +++++++++++++++++++
 1 file changed, 19 insertions(+)
 create mode 100644 mergetools/deltawalker

diff --git a/mergetools/deltawalker b/mergetools/deltawalker
new file mode 100644
index 0000000..f8631f2
--- /dev/null
+++ b/mergetools/deltawalker
@@ -0,0 +1,19 @@
+diff_cmd () {
+	"$merge_tool_path" "$LOCAL" "$REMOTE" >/dev/null 2>&1
+}
+
+merge_cmd () {
+	# The $(pwd)/ in front of $MERGED should not be necessary but
+	# DeltaWalker (at least ver XXX) crashes with a JRE exception
+	# and the $(pwd)/ seems to work around it.
+	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
+}
+
+translate_merge_tool_path() {
+	echo DeltaWalker
+}
-- 
1.7.9.2.390.g8f827

Re: [PATCH v2] mergetools: add support for DeltaWalker

[Tim Henigan]

On Sun, Mar 4, 2012 at 9:10 PM, David Aguilar <davvid@gmail.com> wrote:

> Thanks, that makes a lot of sense.  It looks like they have a bug.  Do
> they know about it?

I have not reported the problem to them yet.


> Can you mention this in a comment so that someone doesn't copy/paste
> it into another tool in the future?

Will add a comment in v4.


> If they fix it then we may want to consider only supporting the newer
> version and remove the $PWD.  Does the OS X version have this bug too?

I don't have a OS X environment to test on.  However, I did just find
out that the bug is not present on Ubuntu 11.10.  On Ubuntu, it works
with or without $(pwd)/.  The two platforms have different Java JDKs,
so that could be a factor.


> I didn't notice any other production shell scripts in git using $PWD.
> There is one use of `pwd` in git-stash.sh and it's used in a few
> tests, as is $PWD, but $(pwd) is the most prevalent overall.  I don't
> know the reason $PWD is avoided in the git shell scripts (though
> portability is often the reason).
>
> Deltawalker is not currently available on any of the platforms where
> this would be a concern, but we may still want to use $(pwd) for
> consistency with the other commands.  Does that work instead?

$(pwd) works...I will make this change in v4.