threads / patch / 29815

v2mergetools: add support for DeltaWalker

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

## tl;dr

8 messages between Mar 2, 2012 and Mar 5, 2012. Diffs are folded; open one to read it.

replies: 7people: 3as markdown or json

Tim Henigan· Mar 2, 2012, 13:27 UTC · lore

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
Show changes to mergetools/DeltaWalker +12 −0
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
Junio C Hamano· Mar 2, 2012, 22:39 UTC · re: Tim Henigan · lore

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

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.

Show 21 quoted lines
>  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
> +}
David Aguilar· Mar 3, 2012, 22:00 UTC · re: Junio C Hamano · lore

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

On Fri, Mar 2, 2012 at 2:39 PM, Junio C Hamano <gitster@pobox.com> wrote:
Show 10 quoted lines
> 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.
Show 24 quoted lines
> 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
Junio C Hamano· Mar 3, 2012, 22:47 UTC · re: David Aguilar · lore

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

David Aguilar <davvid@gmail.com> writes:
Show 11 quoted lines
>>> +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?
Tim Henigan· Mar 4, 2012, 03:50 UTC · re: Junio C Hamano · lore

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

On Sat, Mar 3, 2012 at 5:47 PM, Junio C Hamano <gitster@pobox.com> wrote:
Show 17 quoted lines
> 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
David Aguilar· Mar 5, 2012, 02:10 UTC · re: Tim Henigan · lore

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

On Sat, Mar 3, 2012 at 7:50 PM, Tim Henigan <tim.henigan@gmail.com> wrote:
Show 34 quoted lines
> 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
Junio C Hamano· Mar 5, 2012, 02:38 UTC · re: David Aguilar · lore

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

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
Show changes to mergetools/deltawalker +19 −0
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
Tim Henigan· Mar 5, 2012, 14:22 UTC · re: David Aguilar · lore

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

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.

Show 9 quoted lines
> 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.

← back to recent threads