Re: [PATCH] mergetools: add support for DeltaWalker
On Thu, Mar 1, 2012 at 11:37 PM, Junio C Hamano <gitster@pobox.com> wrote:
Show 6 quoted lines
> 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.
Show 8 quoted lines
> 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
Show 8 quoted lines
>> + 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.
Tim Henigan <tim.henigan@gmail.com>