{"thread":{"id":"29815","subject":"[PATCH v2] mergetools: add support for DeltaWalker","startedAt":"2012-03-02T13:27:47Z","lastAt":"2012-03-05T14:22:05Z","messageCount":8,"participants":["Tim Henigan","Junio C Hamano","David Aguilar"],"isPatch":true,"patchVersion":2,"patchTotal":null},"messages":[{"id":"185919","messageId":"1330694867-7601-1-git-send-email-tim.henigan@gmail.com","threadId":"29815","inReplyTo":null,"subject":"[PATCH v2] mergetools: add support for DeltaWalker","fromName":"Tim Henigan","fromEmail":"tim.henigan@gmail.com","sentAt":"2012-03-02T13:27:47Z","receivedAt":"2012-03-02T13:27:47Z","isPatch":true,"sender":{"key":"tim.henigan@gmail.com","avatar":"https://avatars.githubusercontent.com/u/42022?v=4"},"body":"Thanks to bc7a96a8965d, it is possible to integrate new external\ndiff/merge tools by adding a simple shell script at mergetools/$tool.\n\nThis commit adds DeltaWalker support.\n\nSigned-off-by: Tim Henigan \n---\n\nChanges in v2:\n - reworded the commit message\n - moved >/dev/null redirect to after the final fi statement\n - removed the 'status=$?' line at the end of merge_cmd()\n\nTested with DeltaWalker v1.9.8 on Ubuntu 11.10 and msysgit on Win7.\n\n\n mergetools/DeltaWalker | 12 ++++++++++++\n 1 file changed, 12 insertions(+)\n create mode 100644 mergetools/DeltaWalker\n\ndiff --git a/mergetools/DeltaWalker b/mergetools/DeltaWalker\nnew file mode 100644\nindex 0000000..b9e6618\n--- /dev/null\n+++ b/mergetools/DeltaWalker\n@@ -0,0 +1,12 @@\n+diff_cmd () {\n+\t\"$merge_tool_path\" \"$LOCAL\" \"$REMOTE\" >/dev/null 2>&1\n+}\n+\n+merge_cmd () {\n+\tif $base_present\n+\tthen\n+\t\t\"$merge_tool_path\" \"$LOCAL\" \"$REMOTE\" \"$BASE\" -merged=\"$PWD/$MERGED\"\n+\telse\n+\t\t\"$merge_tool_path\" \"$LOCAL\" \"$REMOTE\" -merged=\"$PWD/$MERGED\"\n+\tfi >/dev/null 2>&1\n+}\n-- \n1.7.9.2.315.g25a78\n"},{"id":"185949","messageId":"7vaa3ybpat.fsf@alter.siamese.dyndns.org","threadId":"29815","inReplyTo":"1330694867-7601-1-git-send-email-tim.henigan@gmail.com","subject":"Re: [PATCH v2] mergetools: add support for DeltaWalker","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-03-02T22:39:38Z","receivedAt":"2012-03-02T22:39:38Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Tim Henigan writes:\n\n> mergetools/DeltaWalker | 12 ++++++++++++\n\nHow does an end user choose to use this backend? Perhaps like this?\n\n $ git mergetool --tool=DeltaWalker\n\nAll the other files in mergetools/ are in lower case, and I _strongly_\nprefer to have this new file also be in lower case.\n\nSuch a change may mean you may have to override translate_merge_tool_path\nin this file, like some other backends seem to do.\n\n> 1 file changed, 12 insertions(+)\n> create mode 100644 mergetools/DeltaWalker\n>\n> diff --git a/mergetools/DeltaWalker b/mergetools/DeltaWalker\n> new file mode 100644\n> index 0000000..b9e6618\n> --- /dev/null\n> +++ b/mergetools/DeltaWalker\n> @@ -0,0 +1,12 @@\n> +diff_cmd () {\n> +\t\"$merge_tool_path\" \"$LOCAL\" \"$REMOTE\" >/dev/null 2>&1\n> +}\n> +\n> +merge_cmd () {\n> +\tif $base_present\n> +\tthen\n> +\t\t\"$merge_tool_path\" \"$LOCAL\" \"$REMOTE\" \"$BASE\" -merged=\"$PWD/$MERGED\"\n> +\telse\n> +\t\t\"$merge_tool_path\" \"$LOCAL\" \"$REMOTE\" -merged=\"$PWD/$MERGED\"\n> +\tfi >/dev/null 2>&1\n> +}\n"},{"id":"186016","messageId":"CAJDDKr4q7gmFeHuGM5hFruduHw-3mGa+CMU=U6X0jjXUsJLsvw@mail.gmail.com","threadId":"29815","inReplyTo":"7vaa3ybpat.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v2] mergetools: add support for DeltaWalker","fromName":"David Aguilar","fromEmail":"davvid@gmail.com","sentAt":"2012-03-03T22:00:17Z","receivedAt":"2012-03-03T22:00:17Z","isPatch":true,"sender":{"key":"davvid@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13196?v=4"},"body":"On Fri, Mar 2, 2012 at 2:39 PM, Junio C Hamano wrote:\n> Tim Henigan writes:\n>\n>>  mergetools/DeltaWalker |   12 ++++++++++++\n>\n> How does an end user choose to use this backend?  Perhaps like this?\n>\n>    $ git mergetool --tool=DeltaWalker\n>\n> All the other files in mergetools/ are in lower case, and I _strongly_\n> prefer to have this new file also be in lower case.\n\nI agree.\n\n> Such a change may mean you may have to override translate_merge_tool_path\n> in this file, like some other backends seem to do.\n>\n>>  1 file changed, 12 insertions(+)\n>>  create mode 100644 mergetools/DeltaWalker\n>>\n>> diff --git a/mergetools/DeltaWalker b/mergetools/DeltaWalker\n>> new file mode 100644\n>> index 0000000..b9e6618\n>> --- /dev/null\n>> +++ b/mergetools/DeltaWalker\n>> @@ -0,0 +1,12 @@\n>> +diff_cmd () {\n>> +     \"$merge_tool_path\" \"$LOCAL\" \"$REMOTE\" >/dev/null 2>&1\n>> +}\n>> +\n>> +merge_cmd () {\n>> +     if $base_present\n>> +     then\n>> +             \"$merge_tool_path\" \"$LOCAL\" \"$REMOTE\" \"$BASE\" -merged=\"$PWD/$MERGED\"\n>> +     else\n>> +             \"$merge_tool_path\" \"$LOCAL\" \"$REMOTE\" -merged=\"$PWD/$MERGED\"\n>> +     fi >/dev/null 2>&1\n>> +}\n\nIs the $PWD/ prefix strictly needed? The rest of the mergetools use\n$MERGED as-is. Does it work without it?\n-- \nDavid\n"},{"id":"186020","messageId":"7vmx7x5ml0.fsf@alter.siamese.dyndns.org","threadId":"29815","inReplyTo":"CAJDDKr4q7gmFeHuGM5hFruduHw-3mGa+CMU=U6X0jjXUsJLsvw@mail.gmail.com","subject":"Re: [PATCH v2] mergetools: add support for DeltaWalker","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-03-03T22:47:07Z","receivedAt":"2012-03-03T22:47:07Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"David Aguilar writes:\n\n>>> +merge_cmd () {\n>>> +     if $base_present\n>>> +     then\n>>> +             \"$merge_tool_path\" \"$LOCAL\" \"$REMOTE\" \"$BASE\" -merged=\"$PWD/$MERGED\"\n>>> +     else\n>>> +             \"$merge_tool_path\" \"$LOCAL\" \"$REMOTE\" -merged=\"$PWD/$MERGED\"\n>>> +     fi >/dev/null 2>&1\n>>> +}\n>\n> Is the $PWD/ prefix strictly needed? The rest of the mergetools use\n> $MERGED as-is. Does it work without it?\n\nHrm, I didn't notice it but they do look fishy. Thanks for good eyes.\n\nTim?\n"},{"id":"186022","messageId":"CAFouetjZ0kQMnAPdNey5kSEmq+fmQqq+fC51hcT+9k6T8c6uOw@mail.gmail.com","threadId":"29815","inReplyTo":"7vmx7x5ml0.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v2] mergetools: add support for DeltaWalker","fromName":"Tim Henigan","fromEmail":"tim.henigan@gmail.com","sentAt":"2012-03-04T03:50:55Z","receivedAt":"2012-03-04T03:50:55Z","isPatch":true,"sender":{"key":"tim.henigan@gmail.com","avatar":"https://avatars.githubusercontent.com/u/42022?v=4"},"body":"On Sat, Mar 3, 2012 at 5:47 PM, Junio C Hamano wrote:\n> David Aguilar writes:\n>\n>>>> +merge_cmd () {\n>>>> +     if $base_present\n>>>> +     then\n>>>> +             \"$merge_tool_path\" \"$LOCAL\" \"$REMOTE\" \"$BASE\" -merged=\"$PWD/$MERGED\"\n>>>> +     else\n>>>> +             \"$merge_tool_path\" \"$LOCAL\" \"$REMOTE\" -merged=\"$PWD/$MERGED\"\n>>>> +     fi >/dev/null 2>&1\n>>>> +}\n>>\n>> Is the $PWD/ prefix strictly needed?  The rest of the mergetools use\n>> $MERGED as-is.  Does it work without it?\n>\n> Hrm, I didn't notice it but they do look fishy.  Thanks for good eyes.\n>\n> Tim?\n\nI ran a quick test using msysgit v1.7.9 on Win7 64-bit and found that\nit fails without '$PWD'.\n\nWhen '$PWD/' is removed from the '-merged' option, it results in a\nJava JRE crash and the conflict resolutions entered by the user are\nnot written to the file.\n\nThe JRE exception is 'EXCEPTION_ACCESS_VIOLATION (0xc0000005)'. I\nposted the full text of the exception to a public location, in case\nanyone is interested [1].\n\nThe format of the '-merged' option was copied directly from the\nDeltaWalker manual. There was no explanation why '$PWD' is needed.\n\n[1]: https://gist.github.com/1970590\n"},{"id":"186062","messageId":"CAJDDKr6toChs8KpnAbjxeemBZT1AT1Uk9Ajk7DWBREuM6yFMzg@mail.gmail.com","threadId":"29815","inReplyTo":"CAFouetjZ0kQMnAPdNey5kSEmq+fmQqq+fC51hcT+9k6T8c6uOw@mail.gmail.com","subject":"Re: [PATCH v2] mergetools: add support for DeltaWalker","fromName":"David Aguilar","fromEmail":"davvid@gmail.com","sentAt":"2012-03-05T02:10:30Z","receivedAt":"2012-03-05T02:10:30Z","isPatch":true,"sender":{"key":"davvid@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13196?v=4"},"body":"On Sat, Mar 3, 2012 at 7:50 PM, Tim Henigan wrote:\n> On Sat, Mar 3, 2012 at 5:47 PM, Junio C Hamano wrote:\n>> David Aguilar writes:\n>>\n>>>>> +merge_cmd () {\n>>>>> +     if $base_present\n>>>>> +     then\n>>>>> +             \"$merge_tool_path\" \"$LOCAL\" \"$REMOTE\" \"$BASE\" -merged=\"$PWD/$MERGED\"\n>>>>> +     else\n>>>>> +             \"$merge_tool_path\" \"$LOCAL\" \"$REMOTE\" -merged=\"$PWD/$MERGED\"\n>>>>> +     fi >/dev/null 2>&1\n>>>>> +}\n>>>\n>>> Is the $PWD/ prefix strictly needed?  The rest of the mergetools use\n>>> $MERGED as-is.  Does it work without it?\n>>\n>> Hrm, I didn't notice it but they do look fishy.  Thanks for good eyes.\n>>\n>> Tim?\n>\n> I ran a quick test using msysgit v1.7.9 on Win7 64-bit and found that\n> it fails without '$PWD'.\n>\n> When '$PWD/' is removed from the '-merged' option, it results in a\n> Java JRE crash and the conflict resolutions entered by the user are\n> not written to the file.\n>\n> The JRE exception is 'EXCEPTION_ACCESS_VIOLATION (0xc0000005)'.  I\n> posted the full text of the exception to a public location, in case\n> anyone is interested [1].\n>\n> The format of the '-merged' option was copied directly from the\n> DeltaWalker manual.  There was no explanation why '$PWD' is needed.\n>\n> [1]: https://gist.github.com/1970590\n\nThanks, that makes a lot of sense. It looks like they have a bug. Do\nthey know about it?\n\nMy naive guess is that they are using some inotify-like thing and\nsubscribing to a directory (per the stack trace). Since\ndirname(\"relative\") is \"\" they crash.\n\nSo this bug probably wouldn't be present when merging a file in a\nsub-directory, or ../ relative path. Is this the case?\n\nCan you mention this in a comment so that someone doesn't copy/paste\nit into another tool in the future?\n\nIf they fix it then we may want to consider only supporting the newer\nversion and remove the $PWD. Does the OS X version have this bug too?\n\nI didn't notice any other production shell scripts in git using $PWD.\nThere is one use of `pwd` in git-stash.sh and it's used in a few\ntests, as is $PWD, but $(pwd) is the most prevalent overall. I don't\nknow the reason $PWD is avoided in the git shell scripts (though\nportability is often the reason).\n\nDeltawalker is not currently available on any of the platforms where\nthis would be a concern, but we may still want to use $(pwd) for\nconsistency with the other commands. Does that work instead?\n-- \nDavid\n"},{"id":"186064","messageId":"7v399n7ox8.fsf@alter.siamese.dyndns.org","threadId":"29815","inReplyTo":"CAJDDKr6toChs8KpnAbjxeemBZT1AT1Uk9Ajk7DWBREuM6yFMzg@mail.gmail.com","subject":"Re: [PATCH v2] mergetools: add support for DeltaWalker","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-03-05T02:38:11Z","receivedAt":"2012-03-05T02:38:11Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"David Aguilar writes:\n\n> Can you mention this in a comment so that someone doesn't copy/paste\n> it into another tool in the future?\n\nOk, I think the result should look like this, but could you fill in the\ndetails in the below?\n\n-- >8 --\nFrom: Tim Henigan \nDate: Sat, 3 Mar 2012 11:56:34 -0500\nSubject: [PATCH] mergetools: add a plug-in to support DeltaWalker\n\nDeltaWalker is a non-free tool that is popular among xxxxxxx users. Add a\nplug-in to support it from difftool and mergetool.\n\nNote that $(pwd)/ in front of $MERGED shouldn't be necessary, but without\nit, DeltaWalker crashes with an JRE exception.\n\nSigned-off-by: Tim Henigan \nHelped-by: David Aguilar\nSigned-off-by: Junio C Hamano \n---\n mergetools/deltawalker | 19 +++++++++++++++++++\n 1 file changed, 19 insertions(+)\n create mode 100644 mergetools/deltawalker\n\ndiff --git a/mergetools/deltawalker b/mergetools/deltawalker\nnew file mode 100644\nindex 0000000..f8631f2\n--- /dev/null\n+++ b/mergetools/deltawalker\n@@ -0,0 +1,19 @@\n+diff_cmd () {\n+\t\"$merge_tool_path\" \"$LOCAL\" \"$REMOTE\" >/dev/null 2>&1\n+}\n+\n+merge_cmd () {\n+\t# The $(pwd)/ in front of $MERGED should not be necessary but\n+\t# DeltaWalker (at least ver XXX) crashes with a JRE exception\n+\t# and the $(pwd)/ seems to work around it.\n+\tif $base_present\n+\tthen\n+\t\t\"$merge_tool_path\" \"$LOCAL\" \"$REMOTE\" \"$BASE\" -merged=\"$(pwd)/$MERGED\"\n+\telse\n+\t\t\"$merge_tool_path\" \"$LOCAL\" \"$REMOTE\" -merged=\"$(pwd)/$MERGED\"\n+\tfi >/dev/null 2>&1\n+}\n+\n+translate_merge_tool_path() {\n+\techo DeltaWalker\n+}\n-- \n1.7.9.2.390.g8f827\n"},{"id":"186115","messageId":"CAFouetgZOgsiA_85N+=CfC5wXOg8PWFJxfw6U0grgs9SYEFsAA@mail.gmail.com","threadId":"29815","inReplyTo":"CAJDDKr6toChs8KpnAbjxeemBZT1AT1Uk9Ajk7DWBREuM6yFMzg@mail.gmail.com","subject":"Re: [PATCH v2] mergetools: add support for DeltaWalker","fromName":"Tim Henigan","fromEmail":"tim.henigan@gmail.com","sentAt":"2012-03-05T14:22:05Z","receivedAt":"2012-03-05T14:22:05Z","isPatch":true,"sender":{"key":"tim.henigan@gmail.com","avatar":"https://avatars.githubusercontent.com/u/42022?v=4"},"body":"On Sun, Mar 4, 2012 at 9:10 PM, David Aguilar wrote:\n\n> Thanks, that makes a lot of sense.  It looks like they have a bug.  Do\n> they know about it?\n\nI have not reported the problem to them yet.\n\n\n> Can you mention this in a comment so that someone doesn't copy/paste\n> it into another tool in the future?\n\nWill add a comment in v4.\n\n\n> If they fix it then we may want to consider only supporting the newer\n> version and remove the $PWD.  Does the OS X version have this bug too?\n\nI don't have a OS X environment to test on. However, I did just find\nout that the bug is not present on Ubuntu 11.10. On Ubuntu, it works\nwith or without $(pwd)/. The two platforms have different Java JDKs,\nso that could be a factor.\n\n\n> I didn't notice any other production shell scripts in git using $PWD.\n> There is one use of `pwd` in git-stash.sh and it's used in a few\n> tests, as is $PWD, but $(pwd) is the most prevalent overall.  I don't\n> know the reason $PWD is avoided in the git shell scripts (though\n> portability is often the reason).\n>\n> Deltawalker is not currently available on any of the platforms where\n> this would be a concern, but we may still want to use $(pwd) for\n> consistency with the other commands.  Does that work instead?\n\n$(pwd) works...I will make this change in v4.\n"}]}