Re: [PATCH] mergetools: add support for DeltaWalker

[Tim Henigan <tim.henigan@gmail.com>]

On Thu, Mar 1, 2012 at 11:37 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Tim Henigan <tim.henigan@gmail.com> 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.