On Fri, Jan 25, 2013 at 10:50:58PM -0800, David Aguilar wrote:
> 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 <dav...@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.

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

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

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

> +             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"
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Reply via email to