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