Re: [PATCH] mergetools: implemented new mergetool file for ExamDiff
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.
Show 6 quoted lines
> 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.
Show 31 quoted lines
> 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_helpersWe 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().
Show 9 quoted lines
> @@ -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 $executableIt might nut hurt to write this as,
printf '%s' "$executable"
For consistency and future-proofing.
Show 23 quoted lines
> + 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.
Show 32 quoted lines
> 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
Jacob Nisnevich
