{"thread":{"id":"29808","subject":"[PATCH] mergetools: add support for DeltaWalker","startedAt":"2012-03-02T01:47:52Z","lastAt":"2012-03-02T13:22:09Z","messageCount":3,"participants":["Tim Henigan","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"185874","messageId":"1330652872-916-1-git-send-email-tim.henigan@gmail.com","threadId":"29808","inReplyTo":null,"subject":"[PATCH] mergetools: add support for DeltaWalker","fromName":"Tim Henigan","fromEmail":"tim.henigan@gmail.com","sentAt":"2012-03-02T01:47:52Z","receivedAt":"2012-03-02T01:47:52Z","isPatch":true,"sender":{"key":"tim.henigan@gmail.com","avatar":"https://avatars.githubusercontent.com/u/42022?v=4"},"body":"Since bc7a96a8965d, integration with external diff/merge tools requires\na mergetools/$tool file to be present.\n\nThis commits adds support for DeltaWalker.\n\nSigned-off-by: Tim Henigan \n---\n\nTested with DeltaWalker v1.9.8 on Ubuntu 11.10 and msysgit on Win7.\n\n\n mergetools/DeltaWalker | 13 +++++++++++++\n 1 file changed, 13 insertions(+)\n create mode 100644 mergetools/DeltaWalker\n\ndiff --git a/mergetools/DeltaWalker b/mergetools/DeltaWalker\nnew file mode 100644\nindex 0000000..ae66686\n--- /dev/null\n+++ b/mergetools/DeltaWalker\n@@ -0,0 +1,13 @@\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\" >/dev/null 2>&1\n+ else\n+ \"$merge_tool_path\" \"$LOCAL\" \"$REMOTE\" -merged=\"$PWD/$MERGED\" >/dev/null 2>&1\n+ fi\n+ status=$?\n+}\n-- \n1.7.9.GIT\n"},{"id":"185881","messageId":"7vpqcvk496.fsf@alter.siamese.dyndns.org","threadId":"29808","inReplyTo":"1330652872-916-1-git-send-email-tim.henigan@gmail.com","subject":"Re: [PATCH] mergetools: add support for DeltaWalker","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-03-02T04:37:09Z","receivedAt":"2012-03-02T04:37:09Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Tim Henigan writes:\n\n> Since bc7a96a8965d, integration with external diff/merge tools requires\n> a mergetools/$tool file to be present.\n>\n> This commits adds support for DeltaWalker.\n>\n> Signed-off-by: Tim Henigan \n> ---\n>\n> Tested with DeltaWalker v1.9.8 on Ubuntu 11.10 and msysgit on Win7.\n>\n>\n> mergetools/DeltaWalker | 13 +++++++++++++\n> 1 file changed, 13 insertions(+)\n> create mode 100644 mergetools/DeltaWalker\n\nI see that the earlier refactoring to make mergetool backend pluggable is\nstarting to pay off rather nicely. It is not \"since ..., requires ...\",\nbut \"thanks to ..., adding a random new tool is just a matter of dropping\na trivial shell snippet in the directory\".\n\n> diff --git a/mergetools/DeltaWalker b/mergetools/DeltaWalker\n> new file mode 100644\n> index 0000000..ae66686\n> --- /dev/null\n> +++ b/mergetools/DeltaWalker\n> @@ -0,0 +1,13 @@\n> +diff_cmd () {\n> + \"$merge_tool_path\" \"$LOCAL\" \"$REMOTE\" >/dev/null 2>&1\n> +}\n\nI noticed that most if not all of the mergetool plug-ins have these ugly\n/dev/null redirects everywhere, and I also suspect that this may be\ninherited from the first bad instance with copy-and-paste. I was about to\nsuggest doing these redirection on the calling side in git-mergetool.sh\nbut there are some that do not want them, so perhaps each plug-in needs to\ndecide if they want redirection individually.\n\n> +merge_cmd () {\n> + if $base_present\n> + then\n> + \"$merge_tool_path\" \"$LOCAL\" \"$REMOTE\" \"$BASE\" -merged=\"$PWD/$MERGED\" >/dev/null 2>&1\n> + else\n> + \"$merge_tool_path\" \"$LOCAL\" \"$REMOTE\" -merged=\"$PWD/$MERGED\" >/dev/null 2>&1\n> + fi\n\nPerhaps doing the above like this might make it a bit less of an eye-sore.\n\n\tif $base_present\n then\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> + status=$?\n\nThis is highly dubious. Looking at existing mergetools/*, I think the\ncaller expects merge_cmd to signal success or failure with $?, so you\nprobably just want to drop this line; the caller will then get the $?\nthat was set by the \"$merge_tool_path\" command.\n\nThat is how your diff_cmd is communicating with its caller after all, no?\n\n> +}\n"},{"id":"185918","messageId":"CAFouethVURpXFDkm8N=riCQDaOYAXYg3VpwKRFaHMftNWr9=dw@mail.gmail.com","threadId":"29808","inReplyTo":"7vpqcvk496.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] mergetools: add support for DeltaWalker","fromName":"Tim Henigan","fromEmail":"tim.henigan@gmail.com","sentAt":"2012-03-02T13:22:09Z","receivedAt":"2012-03-02T13:22:09Z","isPatch":true,"sender":{"key":"tim.henigan@gmail.com","avatar":"https://avatars.githubusercontent.com/u/42022?v=4"},"body":"On Thu, Mar 1, 2012 at 11:37 PM, Junio C Hamano wrote:\n> Tim Henigan writes:\n>\n> I see that the earlier refactoring to make mergetool backend pluggable is\n> starting to pay off rather nicely.  It is not \"since ..., requires ...\",\n> but \"thanks to ..., adding a random new tool is just a matter of dropping\n> a trivial shell snippet in the directory\".\n\nI will reword the commit message in v2.\n\n\n> Perhaps doing the above like this might make it a bit less of an eye-sore.\n>\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\nWill update in v2.\n\n\n>> +       status=$?\n>\n> This is highly dubious.  Looking at existing mergetools/*, I think the\n> caller expects merge_cmd to signal success or failure with $?, so you\n> probably just want to drop this line; the caller will then get the $?\n> that was set by the \"$merge_tool_path\" command.\n>\n> That is how your diff_cmd is communicating with its caller after all, no?\n\nIt seems that only two of the existing scripts use this 'status=$?'\nline (emerge and kdiff3). I used the 'kdiff3' file as my starting\npoint without actually trying to understand why that line was present.\n Local testing shows that I don't need this line for DeltaWalker, so\nit will be removed in v2.\n"}]}