Re: [PATCH] mergetools: add support for DeltaWalker
Tim Henigan <tim.henigan@gmail.com> writes:
Show 14 quoted lines
> 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 <tim.henigan@gmail.com>
> ---
>
> 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".
Show 9 quoted lines
> 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.
Show 7 quoted lines
> +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
> + fiPerhaps 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