Re: [PATCH] mergetools: add support for DeltaWalker

From: Junio C Hamano <gitster@pobox.com>
Date: Mar 2, 2012, 04:37 UTC
Message-ID: <7vpqcvk496.fsf@alter.siamese.dyndns.org>
In-Reply-To: <1330652872-916-1-git-send-email-tim.henigan@gmail.com>

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

> +}