[PATCH v3 2/2] mergetools: Simplify how we handle "vim" and "defaults"

[PATCH v3 2/2] mergetools: Simplify how we handle "vim" and "defaults"

[David Aguilar]

Remove the exceptions for "vim" and "defaults" in the mergetool library
so that every filename in mergetools/ matches 1:1 with the name of a
valid built-in tool.

Make common functions available in $MERGE_TOOLS_DIR/include/.

Signed-off-by: David Aguilar <davvid@gmail.com>
---
v2 used "include" instead of "mergetools/include"
in the Makefile.  Please ignore it.

 Makefile                                     |  6 +++-
 git-mergetool--lib.sh                        | 41 ++++++++--------------------
 mergetools/gvimdiff                          |  1 +
 mergetools/gvimdiff2                         |  1 +
 mergetools/{defaults => include/defaults.sh} |  0
 mergetools/{vim => vimdiff}                  |  0
 mergetools/vimdiff2                          |  1 +
 7 files changed, 19 insertions(+), 31 deletions(-)
 create mode 100644 mergetools/gvimdiff
 create mode 100644 mergetools/gvimdiff2
 rename mergetools/{defaults => include/defaults.sh} (100%)
 rename mergetools/{vim => vimdiff} (100%)
 create mode 100644 mergetools/vimdiff2

diff --git a/Makefile b/Makefile
index f69979e..26f217f 100644
--- a/Makefile
+++ b/Makefile
@@ -2724,7 +2724,11 @@ install: all
 	$(INSTALL) $(install_bindir_programs) '$(DESTDIR_SQ)$(bindir_SQ)'
 	$(MAKE) -C templates DESTDIR='$(DESTDIR_SQ)' install
 	$(INSTALL) -d -m 755 '$(DESTDIR_SQ)$(mergetools_instdir_SQ)'
-	$(INSTALL) -m 644 mergetools/* '$(DESTDIR_SQ)$(mergetools_instdir_SQ)'
+	$(INSTALL) -m 644 $(filter-out mergetools/include,$(wildcard mergetools/*)) \
+		'$(DESTDIR_SQ)$(mergetools_instdir_SQ)'
+	$(INSTALL) -d -m 755 '$(DESTDIR_SQ)$(mergetools_instdir_SQ)/include'
+	$(INSTALL) -m 644 mergetools/include/* \
+		'$(DESTDIR_SQ)$(mergetools_instdir_SQ)/include'
 ifndef NO_GETTEXT
 	$(INSTALL) -d -m 755 '$(DESTDIR_SQ)$(localedir_SQ)'
 	(cd po/build/locale && $(TAR) cf - .) | \
diff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh
index f1bb372..7ea7510 100644
--- a/git-mergetool--lib.sh
+++ b/git-mergetool--lib.sh
@@ -1,5 +1,7 @@
 #!/bin/sh
 # git-mergetool--lib is a library for common merge tool functions
+MERGE_TOOLS_DIR=$(git --exec-path)/mergetools
+
 diff_mode() {
 	test "$TOOL_MODE" = diff
 }
@@ -44,19 +46,9 @@ valid_tool () {
 }
 
 setup_tool () {
-	case "$1" in
-	vim*|gvim*)
-		tool=vim
-		;;
-	*)
-		tool="$1"
-		;;
-	esac
-	mergetools="$(git --exec-path)/mergetools"
+	tool="$1"
 
-	# Load the default definitions
-	. "$mergetools/defaults"
-	if ! test -f "$mergetools/$tool"
+	if ! test -f "$MERGE_TOOLS_DIR/$tool"
 	then
 		# Use a special return code for this case since we want to
 		# source "defaults" even when an explicit tool path is
@@ -65,8 +57,11 @@ setup_tool () {
 		return 2
 	fi
 
+	# Load the default functions
+	. "$MERGE_TOOLS_DIR/include/defaults.sh"
+
 	# Load the redefined functions
-	. "$mergetools/$tool"
+	. "$MERGE_TOOLS_DIR/$tool"
 
 	if merge_mode && ! can_merge
 	then
@@ -194,24 +189,10 @@ list_merge_tool_candidates () {
 show_tool_help () {
 	unavailable= available= LF='
 '
-
-	scriptlets="$(git --exec-path)"/mergetools
-	for i in "$scriptlets"/*
+	for i in "$MERGE_TOOLS_DIR"/*
 	do
-		. "$scriptlets"/defaults
-		. "$i"
-
-		tool="$(basename "$i")"
-		if test "$tool" = "defaults"
-		then
-			continue
-		elif merge_mode && ! can_merge
-		then
-			continue
-		elif diff_mode && ! can_diff
-		then
-			continue
-		fi
+		tool=$(basename "$i")
+		setup_tool "$tool" 2>/dev/null || continue
 
 		merge_tool_path=$(translate_merge_tool_path "$tool")
 		if type "$merge_tool_path" >/dev/null 2>&1
diff --git a/mergetools/gvimdiff b/mergetools/gvimdiff
new file mode 100644
index 0000000..04a5bb0
--- /dev/null
+++ b/mergetools/gvimdiff
@@ -0,0 +1 @@
+. "$MERGE_TOOLS_DIR/vimdiff"
diff --git a/mergetools/gvimdiff2 b/mergetools/gvimdiff2
new file mode 100644
index 0000000..04a5bb0
--- /dev/null
+++ b/mergetools/gvimdiff2
@@ -0,0 +1 @@
+. "$MERGE_TOOLS_DIR/vimdiff"
diff --git a/mergetools/defaults b/mergetools/include/defaults.sh
similarity index 100%
rename from mergetools/defaults
rename to mergetools/include/defaults.sh
diff --git a/mergetools/vim b/mergetools/vimdiff
similarity index 100%
rename from mergetools/vim
rename to mergetools/vimdiff
diff --git a/mergetools/vimdiff2 b/mergetools/vimdiff2
new file mode 100644
index 0000000..04a5bb0
--- /dev/null
+++ b/mergetools/vimdiff2
@@ -0,0 +1 @@
+. "$MERGE_TOOLS_DIR/vimdiff"
-- 
1.8.0.8.gd6b90fb.dirty

Re: [PATCH v3 2/2] mergetools: Simplify how we handle "vim" and "defaults"

[Junio C Hamano]

David Aguilar <davvid@gmail.com> writes:

> @@ -44,19 +46,9 @@ valid_tool () {
>  }
>  
>  setup_tool () {
> -	case "$1" in
> -	vim*|gvim*)
> -		tool=vim
> -		;;
> -	*)
> -		tool="$1"
> -		;;
> -	esac

This part was an eyesore every time I looked at mergetools scripts.
Good riddance.

Is there still other special case like this, or was this the last
one?

Thanks, both of you, for working on this.

Re: [PATCH v3 2/2] mergetools: Simplify how we handle "vim" and "defaults"

[David Aguilar]

On Sat, Jan 26, 2013 at 7:15 PM, Junio C Hamano <gitster@pobox.com> wrote:
> David Aguilar <davvid@gmail.com> writes:
>
>> @@ -44,19 +46,9 @@ valid_tool () {
>>  }
>>
>>  setup_tool () {
>> -     case "$1" in
>> -     vim*|gvim*)
>> -             tool=vim
>> -             ;;
>> -     *)
>> -             tool="$1"
>> -             ;;
>> -     esac
>
> This part was an eyesore every time I looked at mergetools scripts.
> Good riddance.
>
> Is there still other special case like this, or was this the last
> one?
>
> Thanks, both of you, for working on this.

I believe that was the last special case. :-)  It should be easier
to auto-generate a list of tools for use in the documentation now.
That'll be the the next topic I look into.

I have a question. John mentioned that we can replace the use of
"$(..)" with $(..) in shell.

I have a trivial style patches to replace "$(..)" with $(..)
sitting uncommitted in my tree for mergetool--lib.

Grepping the rest of the tree shows that there are many
occurrences of the "$(..)" idiom in the shell code.

Is this a general rule that should be in CodingStyle,
or is it better left as-is?  Are there cases where DQ should
be used around $(..)?  My understanding is "no", but I don't
know whether that is correct.

Doing the documentation stuff is more important IMO so I probably
shouldn't let myself get too distracted by it, though I am curious.
-- 
David

Re: [PATCH v3 2/2] mergetools: Simplify how we handle "vim" and "defaults"

[Junio C Hamano]

David Aguilar <davvid@gmail.com> writes:

> I have a question. John mentioned that we can replace the use of
> "$(..)" with $(..) in shell.

I think it isn't so much about $(...) but more about quoting the RHS
of assignment.

Consider these:

	var="$another_var"
	var="$(command)"
	var="$one_var$two_var"
	var="$another_var$(command)"

all of which _can_ be done without dq around the RHS, because the
result of variable substitution and command substitution will not be
subject to further word splitting.

I however find it easier to read with dq around the latter two, i.e.
substitution and then concatenation of the result of substitution.
The extra dq makes the intent of the author clearer, especially
while reviewing a script written by other people whose understanding
of the syntax I am not sure about ;-).  Between var=$another and
var="$another", the latter is slightly preferrable for the same
reason.

One questionable case is:

	var=$(command "with quoted parameter")

It makes it a bit harder to scan if there is an extra set of dq
around RHS, i.e.

	var="$(command "with quoted parameter")"

That case is easier to read without dq around it, or you could cheat
by doing this:

	var="$(command 'with quoted parameter')"

In any case, as long as the result is correct, I do not have very
strong preference either way.