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

[David Aguilar <davvid@gmail.com>]

On Sat, Jan 26, 2013 at 9:07 PM, David Aguilar <davvid@gmail.com> wrote:
> On Sat, Jan 26, 2013 at 8:57 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> John Keeping <john@keeping.me.uk> writes:
>>
>>> 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.
>>
>> Is 'include' really used for such a purpose?  It only houses defaults.sh
>> as far as I can see.
>>
>> As that defaults.sh file is used only to define trivially empty
>> shell functions, I wonder if it is better to get rid of it, and
>> define these functions in line in git-mergetool--lib.sh.  Such a
>> change would like the attached on top of the entire series.
>
> I think that's much better.

Would you like me to put this together into a proper patch?

You can also squash it in (along with a removal of the
last line of the commit message) if you prefer.


>>  Makefile                       |  6 +-----
>>  git-mergetool--lib.sh          | 24 ++++++++++++++++++++++--
>>  mergetools/include/defaults.sh | 22 ----------------------
>>  3 files changed, 23 insertions(+), 29 deletions(-)
>>
>> diff --git a/Makefile b/Makefile
>> index 26f217f..f69979e 100644
>> --- a/Makefile
>> +++ b/Makefile
>> @@ -2724,11 +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 $(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'
>> +       $(INSTALL) -m 644 mergetools/* '$(DESTDIR_SQ)$(mergetools_instdir_SQ)'
>>  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 7ea7510..1d0fb12 100644
>> --- a/git-mergetool--lib.sh
>> +++ b/git-mergetool--lib.sh
>> @@ -57,8 +57,28 @@ setup_tool () {
>>                 return 2
>>         fi
>>
>> -       # Load the default functions
>> -       . "$MERGE_TOOLS_DIR/include/defaults.sh"
>> +       # Fallback definitions, to be overriden by tools.
>> +       can_merge () {
>> +               return 0
>> +       }
>> +
>> +       can_diff () {
>> +               return 0
>> +       }
>> +
>> +       diff_cmd () {
>> +               status=1
>> +               return $status
>> +       }
>> +
>> +       merge_cmd () {
>> +               status=1
>> +               return $status
>> +       }
>> +
>> +       translate_merge_tool_path () {
>> +               echo "$1"
>> +       }
>>
>>         # Load the redefined functions
>>         . "$MERGE_TOOLS_DIR/$tool"
>> diff --git a/mergetools/include/defaults.sh b/mergetools/include/defaults.sh
>> deleted file mode 100644
>> index 21e63ec..0000000
>> --- a/mergetools/include/defaults.sh
>> +++ /dev/null
>> @@ -1,22 +0,0 @@
>> -# Redefined by builtin tools
>> -can_merge () {
>> -       return 0
>> -}
>> -
>> -can_diff () {
>> -       return 0
>> -}
>> -
>> -diff_cmd () {
>> -       status=1
>> -       return $status
>> -}
>> -
>> -merge_cmd () {
>> -       status=1
>> -       return $status
>> -}
>> -
>> -translate_merge_tool_path () {
>> -       echo "$1"
>> -}



-- 
David