Re: [PATCH] mergetools: add support for DeltaWalker

On Thu, Mar 1, 2012 at 11:37 PM, Junio C Hamano [off-list ref] wrote:

Tim Henigan [off-list ref] 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.

quoted

+       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.