On Sat, Jan 26, 2013 at 9:07 PM, David Aguilar <davvid@xxxxxxxxx> wrote:
> On Sat, Jan 26, 2013 at 8:57 PM, Junio C Hamano <gitster@xxxxxxxxx> wrote:
>> John Keeping <john@xxxxxxxxxxxxx> 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
--
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