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

[David Aguilar <davvid@gmail.com>]

On Sat, Jan 26, 2013 at 4:12 AM, John Keeping <john@keeping.me.uk> wrote:
> 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 <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.
>
>>  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.


I'll reroll this patch with your notes on top of your new patch later today.

Thanks (and thanks for the Makefile hint.. I knew it was wrong! ;-).


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



-- 
David