Re: [PATCH] mergetools: implemented new mergetool file for ExamDiff

[David Aguilar <davvid@gmail.com>]

On Wed, Mar 23, 2016 at 03:55:45PM -0700, Jacob Nisnevich wrote:
> Signed-off-by: Jacob Nisnevich <jacob.nisnevich@gmail.com>
> ---

Please write commit message in an imperative tone.
e.g. "mergetools: add support for ExamDiff" might be a good summary.


>  mergetools/examdiff           | 20 ++++++++++++++++++++
>  mergetools/mergetools_helpers | 24 ++++++++++++++++++++++++
>  mergetools/winmerge           | 23 +++--------------------
>  3 files changed, 47 insertions(+), 20 deletions(-)
>  create mode 100644 mergetools/examdiff
>  create mode 100644 mergetools/mergetools_helpers


This patch is doing two things.
It's probably worth breaking this patch up into two parts.

Patch 1 can add add the new helper function and update
winmerge to use it.

Patch 2 can add the new examdiff helper.


> diff --git a/mergetools/examdiff b/mergetools/examdiff
> new file mode 100644
> index 0000000..c5edd0e
> --- /dev/null
> +++ b/mergetools/examdiff
> @@ -0,0 +1,20 @@
> +. "$MERGE_TOOLS_DIR/mergetools_helpers"
> +
> +diff_cmd () {
> +	"$merge_tool_path" "$LOCAL" "$REMOTE" -nh
> +}
> +
> +merge_cmd () {
> +	touch "$BACKUP"
> +	if $base_present
> +	then
> +		"$merge_tool_path" -merge "$LOCAL" "$BASE" "$REMOTE" -o:"$MERGED" -nh
> +	else
> +		"$merge_tool_path" -merge "$LOCAL" "$REMOTE" -o:"$MERGED" -nh
> +	fi
> +	check_unchanged
> +}
> +
> +translate_merge_tool_path() {
> +	mergetool_find_win32_cmd "ExamDiff.com" "ExamDiff Pro"
> +}
> diff --git a/mergetools/mergetools_helpers b/mergetools/mergetools_helpers
> new file mode 100644
> index 0000000..46ae2d8
> --- /dev/null
> +++ b/mergetools/mergetools_helpers


We can probably do this without introducing a new file.  One
possible home for this is with the rest of the "default"
definitions of the functions in git-mergetool--lib.sh's
setup_tool() function.

But, that hints that we expect tools to override it.

A better place would be as a normal function inside
git-mergetool--lib.sh, which more strongly hints that we do not
expect tools to override mergetool_find_win32_cmd().


> @@ -0,0 +1,24 @@
> +mergetool_find_win32_cmd () {
> +	executable=$1
> +	folder=$2
> +
> +	# Use executable.com if it exists in $PATH
> +	if type -p $executable >/dev/null 2>&1
> +	then
> +		printf $executable


It might nut hurt to write this as,

		printf '%s' "$executable"


For consistency and future-proofing.


> +		return
> +	fi
> +
> +	# Look for executable in the typical locations
> +	for directory in $(env | grep -Ei '^PROGRAM(FILES(\(X86\))?|W6432)=' |
> +		cut -d '=' -f 2- | sort -u)
> +	do
> +		if test -n "$directory" && test -x "$directory/$folder/$executable"
> +		then
> +			printf '%s' "$directory/$folder/$executable"
> +			return
> +		fi
> +	done
> +
> +	printf $executable
> +}
> diff --git a/mergetools/winmerge b/mergetools/winmerge
> index 74a66d4..c785be8 100644
> --- a/mergetools/winmerge
> +++ b/mergetools/winmerge
> @@ -1,3 +1,5 @@
> +. "$MERGE_TOOLS_DIR/mergetools_helpers"
> +


If we move the definition into mergetool--lib then we do not
need to dot-include any files here -- it'll simply be present
and available.


>  diff_cmd () {
>  	"$merge_tool_path" -u -e "$LOCAL" "$REMOTE"
>  	return 0
> @@ -13,24 +15,5 @@ merge_cmd () {
>  }
>  
>  translate_merge_tool_path() {
> -	# Use WinMergeU.exe if it exists in $PATH
> -	if type -p WinMergeU.exe >/dev/null 2>&1
> -	then
> -		printf WinMergeU.exe
> -		return
> -	fi
> -
> -	# Look for WinMergeU.exe in the typical locations
> -	winmerge_exe="WinMerge/WinMergeU.exe"
> -	for directory in $(env | grep -Ei '^PROGRAM(FILES(\(X86\))?|W6432)=' |
> -		cut -d '=' -f 2- | sort -u)
> -	do
> -		if test -n "$directory" && test -x "$directory/$winmerge_exe"
> -		then
> -			printf '%s' "$directory/$winmerge_exe"
> -			return
> -		fi
> -	done
> -
> -	printf WinMergeU.exe
> +	mergetool_find_win32_cmd "WinMergeU.exe" "WinMerge"
>  }
> -- 
> 1.9.1
> 
-- 
David