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

From: John Keeping <john@keeping.me.uk>
Date: Jan 26, 2013, 12:12 UTC
Message-ID: <20130126121202.GH7498@serenity.lan>
In-Reply-To: <1359183058-51835-1-git-send-email-davvid@gmail.com>

On Fri, Jan 25, 2013 at 10:50:58PM -0800, David Aguilar wrote:

Show 19 quoted lines

> 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>
> ---
> 
> diff --git a/Makefile b/Makefile
> index f69979e..3bc6eb5 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -2724,7 +2724,7 @@ 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)'
> +	cp -R mergetools/* '$(DESTDIR_SQ)$(mergetools_instdir_SQ)'

Shouldn't this be more like this?

	$(INSTALL) -m 644 $(subst 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'

I'm not sure creating an 'include' directory actually buys us much over declaring that 'vimdiff' is the real script and the others just source it. Either way there is a single special entry in the mergetools directory.

Show 29 quoted lines

>  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 aa38bd1..c866ed8 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,25 +46,14 @@ valid_tool () {
>  }
>  
>  setup_tool () {
> -	case "$1" in
> -	vim*|gvim*)
> -		tool=vim
> -		;;
> -	*)
> -		tool="$1"
> -		;;
> -	esac
> -	mergetools="$(git --exec-path)/mergetools"
> +	tool="$1"

Unnecessary quoting.

Show 21 quoted lines

> -	# Load the default definitions
> -	. "$mergetools/defaults"
> -	if ! test -f "$mergetools/$tool"
> +	if ! test -f "$MERGE_TOOLS_DIR/$tool"
>  	then
>  		return 1
>  	fi
> -
> -	# Load the redefined functions
> -	. "$mergetools/$tool"
> +	. "$MERGE_TOOLS_DIR/include/defaults.sh"
> +	. "$MERGE_TOOLS_DIR/$tool"
>  
>  	if merge_mode && ! can_merge
>  	then
> @@ -99,7 +90,7 @@ run_merge_tool () {
>  	base_present="$2"
>  	status=0
>  
> -	# Bring tool-specific functions into scope
> +	# Bring tool specific functions into scope

This isn't related to this change (and I think tool-specific is more correct anyway).

Show 33 quoted lines

>  	setup_tool "$1"
>  
>  	if merge_mode
> @@ -177,18 +168,17 @@ 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"
> +		if test -d "$i"
>  		then
>  			continue
> -		elif merge_mode && ! can_merge
> +		fi
> +
> +		. "$MERGE_TOOLS_DIR"/include/defaults.sh
> +		. "$i"
> +
> +		if merge_mode && ! can_merge
>  		then
>  			continue
>  		elif diff_mode && ! can_diff
> @@ -196,6 +186,7 @@ show_tool_help () {
>  			continue
>  		fi

I'd prefer to see my change to setup_tool done before this so that we can just use:

	setup_tool "$tool" 2>/dev/null || continue

for the above block.  I'll send a fixed version in a couple of minutes.

Show 40 quoted lines

> +		tool=$(basename "$i")
>  		merge_tool_path=$(translate_merge_tool_path "$tool")
>  		if type "$merge_tool_path" >/dev/null 2>&1
>  		then
> diff --git a/mergetools/gvimdiff b/mergetools/gvimdiff
> new file mode 100644
> index 0000000..f5890b1
> --- /dev/null
> +++ b/mergetools/gvimdiff
> @@ -0,0 +1 @@
> +. "$MERGE_TOOLS_DIR/include/vim.sh"
> diff --git a/mergetools/gvimdiff2 b/mergetools/gvimdiff2
> new file mode 100644
> index 0000000..f5890b1
> --- /dev/null
> +++ b/mergetools/gvimdiff2
> @@ -0,0 +1 @@
> +. "$MERGE_TOOLS_DIR/include/vim.sh"
> 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/include/vim.sh
> similarity index 100%
> rename from mergetools/vim
> rename to mergetools/include/vim.sh
> diff --git a/mergetools/vimdiff b/mergetools/vimdiff
> new file mode 100644
> index 0000000..f5890b1
> --- /dev/null
> +++ b/mergetools/vimdiff
> @@ -0,0 +1 @@
> +. "$MERGE_TOOLS_DIR/include/vim.sh"
> diff --git a/mergetools/vimdiff2 b/mergetools/vimdiff2
> new file mode 100644
> index 0000000..f5890b1
> --- /dev/null
> +++ b/mergetools/vimdiff2
> @@ -0,0 +1 @@
> +. "$MERGE_TOOLS_DIR/include/vim.sh"