threads / patch / 32737

v3, 2 partsmergetools: Simplify how we handle "vim" and "defaults"

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

## tl;dr

4 messages between Jan 27, 2013 and Jan 27, 2013. Diffs are folded; open one to read it.

replies: 3people: 2as markdown or json

David Aguilar· Jan 27, 2013, 00:46 UTC · lore

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
Show changes to 7 files +19 −31

Makefile, git-mergetool--lib.sh, mergetools/gvimdiff, mergetools/gvimdiff2, mergetools/defaults, mergetools/vim, 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
Junio C Hamano· Jan 27, 2013, 03:15 UTC · re: David Aguilar · lore

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

David Aguilar <davvid@gmail.com> writes:
Show 12 quoted lines
> @@ -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.
David Aguilar· Jan 27, 2013, 03:56 UTC · re: Junio C Hamano · lore

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

On Sat, Jan 26, 2013 at 7:15 PM, Junio C Hamano <gitster@pobox.com> wrote:
Show 22 quoted lines
> 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
Junio C Hamano· Jan 27, 2013, 04:25 UTC · re: David Aguilar · lore

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

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.

← back to recent threads