[PATCH] mergetools: created new mergetool file for ExamDiff

[PATCH] mergetools: created new mergetool file for ExamDiff

[Jacob Nisnevich]

---
 mergetools/examdiff | 37 +++++++++++++++++++++++++++++++++++++
 1 file changed, 37 insertions(+)
 create mode 100644 mergetools/examdiff

diff --git a/mergetools/examdiff b/mergetools/examdiff
new file mode 100644
index 0000000..474fffe
--- /dev/null
+++ b/mergetools/examdiff
@@ -0,0 +1,37 @@
+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() {
+	# Use ExamDiff.com if it exists in $PATH
+	if type -p ExamDiff.com >/dev/null 2>&1
+	then
+		printf ExamDiff.com
+		return
+	fi
+
+	# Look for ExamDiff.com in the typical locations
+	examdiff="ExamDiff Pro/ExamDiff.com"
+	for directory in $(env | grep -Ei '^PROGRAM(FILES(\(X86\))?|W6432)=' |
+		cut -d '=' -f 2- | sort -u)
+	do
+		if test -n "$directory" && test -x "$directory/$examdiff"
+		then
+			printf '%s' "$directory/$examdiff"
+			return
+		fi
+	done
+
+	printf ExamDiff.com
+}
\ No newline at end of file
-- 
1.9.1

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

[Junio C Hamano]

Jacob Nisnevich <jacob.nisnevich@gmail.com> writes:

> ---

Missing sign-off.

I'll Cc the area expert (David Aguilar).

>  mergetools/examdiff | 37 +++++++++++++++++++++++++++++++++++++
>  1 file changed, 37 insertions(+)
>  create mode 100644 mergetools/examdiff
>
> diff --git a/mergetools/examdiff b/mergetools/examdiff
> new file mode 100644
> index 0000000..474fffe
> --- /dev/null
> +++ b/mergetools/examdiff
> @@ -0,0 +1,37 @@
> +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() {
> +	# Use ExamDiff.com if it exists in $PATH
> +	if type -p ExamDiff.com >/dev/null 2>&1
> +	then
> +		printf ExamDiff.com
> +		return
> +	fi
> +
> +	# Look for ExamDiff.com in the typical locations
> +	examdiff="ExamDiff Pro/ExamDiff.com"
> +	for directory in $(env | grep -Ei '^PROGRAM(FILES(\(X86\))?|W6432)=' |
> +		cut -d '=' -f 2- | sort -u)
> +	do
> +		if test -n "$directory" && test -x "$directory/$examdiff"
> +		then
> +			printf '%s' "$directory/$examdiff"
> +			return
> +		fi
> +	done
> +
> +	printf ExamDiff.com

This complicated heuristics look like a cut-and-paste from the
neighbouring winmerge; makes me suspect that they should share the
same helper function to implement the bulk of the above code for
better maintainability (e.g. imagine in the future Microsoft decides
to introduce another directory organization and makes it necessary
to tweak the pattern you give to 'grep -Ei'---WinMergeU user may
notice that and fix it, while this script will be overlooked and
will stay stale until somebody from examdiff camp do the same fix
later).


> +}
> \ No newline at end of file

No newline at end of file?

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

[David Aguilar]

On Sun, Mar 20, 2016 at 06:02:55PM -0700, Junio C Hamano wrote:
> Jacob Nisnevich <jacob.nisnevich@gmail.com> writes:
> 
> > ---
> 
> Missing sign-off.
> 
> I'll Cc the area expert (David Aguilar).
> 
> >  mergetools/examdiff | 37 +++++++++++++++++++++++++++++++++++++
> >  1 file changed, 37 insertions(+)
> >  create mode 100644 mergetools/examdiff
> >
> > diff --git a/mergetools/examdiff b/mergetools/examdiff
> > new file mode 100644
> > index 0000000..474fffe
> > --- /dev/null
> > +++ b/mergetools/examdiff
> > @@ -0,0 +1,37 @@
> > +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() {
> > +	# Use ExamDiff.com if it exists in $PATH
> > +	if type -p ExamDiff.com >/dev/null 2>&1
> > +	then
> > +		printf ExamDiff.com
> > +		return
> > +	fi
> > +
> > +	# Look for ExamDiff.com in the typical locations
> > +	examdiff="ExamDiff Pro/ExamDiff.com"
> > +	for directory in $(env | grep -Ei '^PROGRAM(FILES(\(X86\))?|W6432)=' |
> > +		cut -d '=' -f 2- | sort -u)
> > +	do
> > +		if test -n "$directory" && test -x "$directory/$examdiff"
> > +		then
> > +			printf '%s' "$directory/$examdiff"
> > +			return
> > +		fi
> > +	done
> > +
> > +	printf ExamDiff.com
> 
> This complicated heuristics look like a cut-and-paste from the
> neighbouring winmerge; makes me suspect that they should share the
> same helper function to implement the bulk of the above code for
> better maintainability (e.g. imagine in the future Microsoft decides
> to introduce another directory organization and makes it necessary
> to tweak the pattern you give to 'grep -Ei'---WinMergeU user may
> notice that and fix it, while this script will be overlooked and
> will stay stale until somebody from examdiff camp do the same fix
> later).

I agree with that.

Something like mergetool_find_win32_cmd() might make sense as a
helper function that we can reuse here.

> > +}
> > \ No newline at end of file
> 
> No newline at end of file?

Using sublime text perhaps?
It defaults to not including the final line newline terminator.

https://forum.sublimetext.com/t/make-saving-newline-at-eof-the-installation-default/9842

If so, please configure it as detailed in the above thread.

cheers,
-- 
David

[PATCH 0/2] Helper function for ExamDiff and Winmerge mergetools

[Jacob Nisnevich]

I've been trying to implement a helper function for both Winmerge and ExamDiff
as you requested, but for some reason including another shell script with . or
source seems to fail on Windows in this case. I've attached the patches for the
two commits I've made so far. Is there anything I'm doing wrong or is it an
issue with Windows?

Jacob Nisnevich (2):
  mergetools: created new mergetool file for ExamDiff
  created helper function for both winmerge and examdiff mergetools

 ...s-created-new-mergetool-file-for-ExamDiff.patch | 58 ++++++++++++++++++++++
 mergetools/examdiff                                | 20 ++++++++
 mergetools/mergetools_helpers                      | 30 +++++++++++
 mergetools/winmerge                                | 23 ++-------
 4 files changed, 111 insertions(+), 20 deletions(-)
 create mode 100644 0001-mergetools-created-new-mergetool-file-for-ExamDiff.patch
 create mode 100644 mergetools/examdiff
 create mode 100644 mergetools/mergetools_helpers

-- 
1.9.1

[PATCH 1/2] mergetools: created new mergetool file for ExamDiff

[Jacob Nisnevich]

Signed-off-by: Jacob Nisnevich <jacob.nisnevich@gmail.com>
---
 mergetools/examdiff | 37 +++++++++++++++++++++++++++++++++++++
 1 file changed, 37 insertions(+)
 create mode 100644 mergetools/examdiff

diff --git a/mergetools/examdiff b/mergetools/examdiff
new file mode 100644
index 0000000..474fffe
--- /dev/null
+++ b/mergetools/examdiff
@@ -0,0 +1,37 @@
+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() {
+	# Use ExamDiff.com if it exists in $PATH
+	if type -p ExamDiff.com >/dev/null 2>&1
+	then
+		printf ExamDiff.com
+		return
+	fi
+
+	# Look for ExamDiff.com in the typical locations
+	examdiff="ExamDiff Pro/ExamDiff.com"
+	for directory in $(env | grep -Ei '^PROGRAM(FILES(\(X86\))?|W6432)=' |
+		cut -d '=' -f 2- | sort -u)
+	do
+		if test -n "$directory" && test -x "$directory/$examdiff"
+		then
+			printf '%s' "$directory/$examdiff"
+			return
+		fi
+	done
+
+	printf ExamDiff.com
+}
\ No newline at end of file
-- 
1.9.1

[PATCH 2/2] created helper function for both winmerge and examdiff mergetools

[Jacob Nisnevich]

Signed-off-by: Jacob Nisnevich <jacob.nisnevich@gmail.com>
---
 ...s-created-new-mergetool-file-for-ExamDiff.patch | 58 ++++++++++++++++++++++
 mergetools/examdiff                                | 25 ++--------
 mergetools/mergetools_helpers                      | 30 +++++++++++
 mergetools/winmerge                                | 23 ++-------
 4 files changed, 95 insertions(+), 41 deletions(-)
 create mode 100644 0001-mergetools-created-new-mergetool-file-for-ExamDiff.patch
 create mode 100644 mergetools/mergetools_helpers

diff --git a/0001-mergetools-created-new-mergetool-file-for-ExamDiff.patch b/0001-mergetools-created-new-mergetool-file-for-ExamDiff.patch
new file mode 100644
index 0000000..99e0d6b
--- /dev/null
+++ b/0001-mergetools-created-new-mergetool-file-for-ExamDiff.patch
@@ -0,0 +1,58 @@
+From a26ad589bc5e747962359c89c5536858748e5eb8 Mon Sep 17 00:00:00 2001
+From: Jacob Nisnevich <jacob.nisnevich@gmail.com>
+Date: Sat, 19 Mar 2016 17:31:50 -0700
+Subject: [PATCH] mergetools: created new mergetool file for ExamDiff
+To: git@vger.kernel.org
+
+---
+ mergetools/examdiff | 37 +++++++++++++++++++++++++++++++++++++
+ 1 file changed, 37 insertions(+)
+ create mode 100644 mergetools/examdiff
+
+diff --git a/mergetools/examdiff b/mergetools/examdiff
+new file mode 100644
+index 0000000..474fffe
+--- /dev/null
++++ b/mergetools/examdiff
+@@ -0,0 +1,37 @@
++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() {
++	# Use ExamDiff.com if it exists in $PATH
++	if type -p ExamDiff.com >/dev/null 2>&1
++	then
++		printf ExamDiff.com
++		return
++	fi
++
++	# Look for ExamDiff.com in the typical locations
++	examdiff="ExamDiff Pro/ExamDiff.com"
++	for directory in $(env | grep -Ei '^PROGRAM(FILES(\(X86\))?|W6432)=' |
++		cut -d '=' -f 2- | sort -u)
++	do
++		if test -n "$directory" && test -x "$directory/$examdiff"
++		then
++			printf '%s' "$directory/$examdiff"
++			return
++		fi
++	done
++
++	printf ExamDiff.com
++}
+\ No newline at end of file
+-- 
+1.9.1
+
diff --git a/mergetools/examdiff b/mergetools/examdiff
index 474fffe..8b66c17 100644
--- a/mergetools/examdiff
+++ b/mergetools/examdiff
@@ -1,3 +1,5 @@
+. mergetools_helpers
+
 diff_cmd () {
 	"$merge_tool_path" "$LOCAL" "$REMOTE" -nh
 }
@@ -14,24 +16,5 @@ merge_cmd () {
 }
 
 translate_merge_tool_path() {
-	# Use ExamDiff.com if it exists in $PATH
-	if type -p ExamDiff.com >/dev/null 2>&1
-	then
-		printf ExamDiff.com
-		return
-	fi
-
-	# Look for ExamDiff.com in the typical locations
-	examdiff="ExamDiff Pro/ExamDiff.com"
-	for directory in $(env | grep -Ei '^PROGRAM(FILES(\(X86\))?|W6432)=' |
-		cut -d '=' -f 2- | sort -u)
-	do
-		if test -n "$directory" && test -x "$directory/$examdiff"
-		then
-			printf '%s' "$directory/$examdiff"
-			return
-		fi
-	done
-
-	printf ExamDiff.com
-}
\ No newline at end of file
+	mergetool_find_win32_cmd "ExamDiff.com" "ExamDiff Pro"
+}
diff --git a/mergetools/mergetools_helpers b/mergetools/mergetools_helpers
new file mode 100644
index 0000000..6df9d09
--- /dev/null
+++ b/mergetools/mergetools_helpers
@@ -0,0 +1,30 @@
+# Find path to win32 executable using typical locations
+# Arguments
+#  executable      - default name of executable file
+#  folder          - folder containing executable file from Program Files
+# Returns
+#  Path to the executable
+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
+		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..265d853 100644
--- a/mergetools/winmerge
+++ b/mergetools/winmerge
@@ -1,3 +1,5 @@
+. mergetools_helpers
+
 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

Re: [PATCH 2/2] created helper function for both winmerge and examdiff mergetools

[Junio C Hamano]

Jacob Nisnevich <jacob.nisnevich@gmail.com> writes:

> diff --git a/mergetools/examdiff b/mergetools/examdiff
> index 474fffe..8b66c17 100644
> --- a/mergetools/examdiff
> +++ b/mergetools/examdiff
> @@ -1,3 +1,5 @@
> +. mergetools_helpers
> +

The way dot-inclusion is done in existing mergetools/* files may
give you a hint, perhaps?

$ git grep '^\. ' mergetools
mergetools/bc3:. "$MERGE_TOOLS_DIR/bc"
mergetools/gvimdiff:. "$MERGE_TOOLS_DIR/vimdiff"
mergetools/gvimdiff2:. "$MERGE_TOOLS_DIR/vimdiff"
mergetools/gvimdiff3:. "$MERGE_TOOLS_DIR/vimdiff"
mergetools/vimdiff2:. "$MERGE_TOOLS_DIR/vimdiff"
mergetools/vimdiff3:. "$MERGE_TOOLS_DIR/vimdiff"

[PATCH] mergetools: implemented new mergetool file for ExamDiff

[Jacob Nisnevich]

Thanks for the hint. I used the other examples to fix the implementation of the 
ExamDiff mergetool. I've tested the Winmerge and ExamDiff mergetools with both
`git difftool` and `git mergetool` and both seem to work as expected. I've 
combined all my changes into one patch. Are there any other issues with my
changes?

Jacob Nisnevich (1):
  mergetools: implemented new mergetool file for ExamDiff

 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

-- 
1.9.1

[PATCH] mergetools: implemented new mergetool file for ExamDiff

[Jacob Nisnevich]

Signed-off-by: Jacob Nisnevich <jacob.nisnevich@gmail.com>
---
 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

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
@@ -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
+		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"
+
 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

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

[David Aguilar]

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

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

[Junio C Hamano]

David Aguilar <davvid@gmail.com> writes:

> 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().

Thanks for a clearly described set of rules that people can easily
follow.

[PATCH 0/2] add support for ExamDiff

[Jacob Nisnevich]

I implemented all the changes you suggested and separated the patch into two
different patches. Thanks for all the help David.

Jacob Nisnevich (2):
  mergetools: create mergetool_find_win32_cmd() helper function for
    winmerge
  mergetools: add support for ExamDiff

 git-mergetool--lib.sh | 25 +++++++++++++++++++++++++
 mergetools/examdiff   | 18 ++++++++++++++++++
 mergetools/winmerge   | 21 +--------------------
 3 files changed, 44 insertions(+), 20 deletions(-)
 create mode 100644 mergetools/examdiff

-- 
1.9.1

[PATCH 1/2] mergetools: create mergetool_find_win32_cmd() helper function for winmerge

[Jacob Nisnevich]

Signed-off-by: Jacob Nisnevich <jacob.nisnevich@gmail.com>
---
 git-mergetool--lib.sh | 25 +++++++++++++++++++++++++
 mergetools/winmerge   | 21 +--------------------
 2 files changed, 26 insertions(+), 20 deletions(-)

diff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh
index 54ac8e4..c5fa820 100644
--- a/git-mergetool--lib.sh
+++ b/git-mergetool--lib.sh
@@ -372,3 +372,28 @@ get_merge_tool () {
 	fi
 	echo "$merge_tool"
 }
+
+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 '%s' $executable
+		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 '%s' $executable
+}
diff --git a/mergetools/winmerge b/mergetools/winmerge
index 74a66d4..f3819d3 100644
--- a/mergetools/winmerge
+++ b/mergetools/winmerge
@@ -13,24 +13,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

[PATCH 2/2] mergetools: add support for ExamDiff

[Jacob Nisnevich]

Signed-off-by: Jacob Nisnevich <jacob.nisnevich@gmail.com>
---
 mergetools/examdiff | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)
 create mode 100644 mergetools/examdiff

diff --git a/mergetools/examdiff b/mergetools/examdiff
new file mode 100644
index 0000000..7b524d4
--- /dev/null
+++ b/mergetools/examdiff
@@ -0,0 +1,18 @@
+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"
+}
-- 
1.9.1

Re: [PATCH 1/2] mergetools: create mergetool_find_win32_cmd() helper function for winmerge

[Junio C Hamano]

Jacob Nisnevich <jacob.nisnevich@gmail.com> writes:

> Signed-off-by: Jacob Nisnevich <jacob.nisnevich@gmail.com>
> ---
>  git-mergetool--lib.sh | 25 +++++++++++++++++++++++++
>  mergetools/winmerge   | 21 +--------------------
>  2 files changed, 26 insertions(+), 20 deletions(-)
>
> diff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh
> index 54ac8e4..c5fa820 100644
> --- a/git-mergetool--lib.sh
> +++ b/git-mergetool--lib.sh
> @@ -372,3 +372,28 @@ get_merge_tool () {
>  	fi
>  	echo "$merge_tool"
>  }
> +
> +mergetool_find_win32_cmd () {
> +	executable=$1
> +	folder=$2
> +
> +	# Use executable.com if it exists in $PATH

s/executable.com/$executable/

> +	if type -p $executable >/dev/null 2>&1

This needs to be quoted,

	if type -p "$executable" >/dev/null 2>&1

> +	then
> +		printf '%s' $executable

Likewise.

> +		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 '%s' $executable

Likewise.

Other than these points, I do not see anything wrong in this patch.
Thanks.

By the way, "directory/folder/stuff" sounds somewhat strange, no?

[PATCH 0/2] mergetools: add support for ExamDiff

[Jacob Nisnevich]

OK I add the quotes and modified the comment. I also changed $folder to 
$sub_directory. I think that makes a little bit more sense and sounds a lot
better.

Jacob Nisnevich (2):
  mergetools: create mergetool_find_win32_cmd() helper function for
    winmerge
  mergetools: add support for ExamDiff

 git-mergetool--lib.sh | 25 +++++++++++++++++++++++++
 mergetools/examdiff   | 18 ++++++++++++++++++
 mergetools/winmerge   | 21 +--------------------
 3 files changed, 44 insertions(+), 20 deletions(-)
 create mode 100644 mergetools/examdiff

-- 
1.9.1

[PATCH 1/2] mergetools: create mergetool_find_win32_cmd() helper function for winmerge

[Jacob Nisnevich]

Signed-off-by: Jacob Nisnevich <jacob.nisnevich@gmail.com>
---
 git-mergetool--lib.sh | 25 +++++++++++++++++++++++++
 mergetools/winmerge   | 21 +--------------------
 2 files changed, 26 insertions(+), 20 deletions(-)

diff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh
index 54ac8e4..302c56d 100644
--- a/git-mergetool--lib.sh
+++ b/git-mergetool--lib.sh
@@ -372,3 +372,28 @@ get_merge_tool () {
 	fi
 	echo "$merge_tool"
 }
+
+mergetool_find_win32_cmd () {
+	executable=$1
+	sub_directory=$2
+
+	# Use $executable if it exists in $PATH
+	if type -p "$executable" >/dev/null 2>&1
+	then
+		printf '%s' "$executable"
+		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/$sub_directory/$executable"
+		then
+			printf '%s' "$directory/$sub_directory/$executable"
+			return
+		fi
+	done
+
+	printf '%s' "$executable"
+}
diff --git a/mergetools/winmerge b/mergetools/winmerge
index 74a66d4..f3819d3 100644
--- a/mergetools/winmerge
+++ b/mergetools/winmerge
@@ -13,24 +13,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

[PATCH 2/2] mergetools: add support for ExamDiff

[Jacob Nisnevich]

Signed-off-by: Jacob Nisnevich <jacob.nisnevich@gmail.com>
---
 mergetools/examdiff | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)
 create mode 100644 mergetools/examdiff

diff --git a/mergetools/examdiff b/mergetools/examdiff
new file mode 100644
index 0000000..7b524d4
--- /dev/null
+++ b/mergetools/examdiff
@@ -0,0 +1,18 @@
+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"
+}
-- 
1.9.1

Re: [PATCH 0/2] mergetools: add support for ExamDiff

[Junio C Hamano]

Jacob Nisnevich <jacob.nisnevich@gmail.com> writes:

> OK I add the quotes and modified the comment. I also changed $folder to 
> $sub_directory. I think that makes a little bit more sense and sounds a lot
> better.
>
> Jacob Nisnevich (2):
>   mergetools: create mergetool_find_win32_cmd() helper function for
>     winmerge
>   mergetools: add support for ExamDiff
>
>  git-mergetool--lib.sh | 25 +++++++++++++++++++++++++
>  mergetools/examdiff   | 18 ++++++++++++++++++
>  mergetools/winmerge   | 21 +--------------------
>  3 files changed, 44 insertions(+), 20 deletions(-)
>  create mode 100644 mergetools/examdiff

This round looked good to me.
David, does this look sensible to you?

Thanks.

Re: [PATCH 0/2] mergetools: add support for ExamDiff

[David Aguilar]

On Fri, Apr 01, 2016 at 11:10:56AM -0700, Junio C Hamano wrote:
> Jacob Nisnevich <jacob.nisnevich@gmail.com> writes:
> 
> > OK I add the quotes and modified the comment. I also changed $folder to 
> > $sub_directory. I think that makes a little bit more sense and sounds a lot
> > better.
> >
> > Jacob Nisnevich (2):
> >   mergetools: create mergetool_find_win32_cmd() helper function for
> >     winmerge
> >   mergetools: add support for ExamDiff
> >
> >  git-mergetool--lib.sh | 25 +++++++++++++++++++++++++
> >  mergetools/examdiff   | 18 ++++++++++++++++++
> >  mergetools/winmerge   | 21 +--------------------
> >  3 files changed, 44 insertions(+), 20 deletions(-)
> >  create mode 100644 mergetools/examdiff
> 
> This round looked good to me.
> David, does this look sensible to you?
> 
> Thanks.

Yes, thank you.

Acked-by: David Aguilar <davvid@gmail.com>

cheers,
-- 
David