On Sat, Jan 26, 2013 at 4:12 AM, John Keeping <john@xxxxxxxxxxxxx> 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@xxxxxxxxx>
>> ---
>>
>> 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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html