# [PATCH] mergetools: add support for DeltaWalker 3 messages from 2012-03-02 to 2012-03-02. Participants: Tim Henigan, Junio C Hamano. Thread: https://gitlist.dev/t/29808 ## Tim Henigan, 2012-03-02 01:47 Subject: [PATCH] mergetools: add support for DeltaWalker Message-ID: <1330652872-916-1-git-send-email-tim.henigan@gmail.com> URL: https://gitlist.dev/e/1330652872-916-1-git-send-email-tim.henigan%40gmail.com ``` 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 --- 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 ``` ## Junio C Hamano, 2012-03-02 04:37 Subject: Re: [PATCH] mergetools: add support for DeltaWalker Message-ID: <7vpqcvk496.fsf@alter.siamese.dyndns.org> URL: https://gitlist.dev/e/7vpqcvk496.fsf%40alter.siamese.dyndns.org In-Reply-To: <1330652872-916-1-git-send-email-tim.henigan@gmail.com> ``` Tim Henigan 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 > --- > > 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? > +} ``` ## Tim Henigan, 2012-03-02 13:22 Subject: Re: [PATCH] mergetools: add support for DeltaWalker Message-ID: URL: https://gitlist.dev/e/CAFouethVURpXFDkm8N%3DriCQDaOYAXYg3VpwKRFaHMftNWr9%3Ddw%40mail.gmail.com In-Reply-To: <7vpqcvk496.fsf@alter.siamese.dyndns.org> ``` On Thu, Mar 1, 2012 at 11:37 PM, Junio C Hamano wrote: > Tim Henigan 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. ```