* [PATCH] mergetools: Simplify how we handle "vim" and "defaults"
@ 2013-01-26 6:50 David Aguilar
2013-01-26 12:12 ` John Keeping
0 siblings, 1 reply; 8+ messages in thread
From: David Aguilar @ 2013-01-26 6:50 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, John Keeping
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>
---
This should make things ok on platforms where we don't have symlinks.
Makefile | 2 +-
git-mergetool--lib.sh | 41 +++++++++++-----------------
mergetools/gvimdiff | 1 +
mergetools/gvimdiff2 | 1 +
mergetools/{defaults => include/defaults.sh} | 0
mergetools/{vim => include/vim.sh} | 0
mergetools/vimdiff | 1 +
mergetools/vimdiff2 | 1 +
8 files changed, 21 insertions(+), 26 deletions(-)
create mode 100644 mergetools/gvimdiff
create mode 100644 mergetools/gvimdiff2
rename mergetools/{defaults => include/defaults.sh} (100%)
rename mergetools/{vim => include/vim.sh} (100%)
create mode 100644 mergetools/vimdiff
create mode 100644 mergetools/vimdiff2
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)'
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"
- # 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
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
+ 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"
--
1.8.0.6.ge6f188f.dirty
^ permalink raw reply related [flat|nested] 8+ messages in thread* Re: [PATCH] mergetools: Simplify how we handle "vim" and "defaults"
2013-01-26 6:50 [PATCH] mergetools: Simplify how we handle "vim" and "defaults" David Aguilar
@ 2013-01-26 12:12 ` John Keeping
2013-01-26 20:40 ` David Aguilar
2013-01-27 4:57 ` Junio C Hamano
0 siblings, 2 replies; 8+ messages in thread
From: John Keeping @ 2013-01-26 12:12 UTC (permalink / raw)
To: David Aguilar; +Cc: Junio C Hamano, git
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.
> + 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"
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH] mergetools: Simplify how we handle "vim" and "defaults"
2013-01-26 12:12 ` John Keeping
@ 2013-01-26 20:40 ` David Aguilar
2013-01-26 20:54 ` John Keeping
2013-01-27 4:57 ` Junio C Hamano
1 sibling, 1 reply; 8+ messages in thread
From: David Aguilar @ 2013-01-26 20:40 UTC (permalink / raw)
To: John Keeping; +Cc: Junio C Hamano, git
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
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH] mergetools: Simplify how we handle "vim" and "defaults"
2013-01-26 20:40 ` David Aguilar
@ 2013-01-26 20:54 ` John Keeping
0 siblings, 0 replies; 8+ messages in thread
From: John Keeping @ 2013-01-26 20:54 UTC (permalink / raw)
To: David Aguilar; +Cc: Junio C Hamano, git
On Sat, Jan 26, 2013 at 12:40:23PM -0800, David Aguilar wrote:
> 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:
> >> 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.
>
> Thanks (and thanks for the Makefile hint.. I knew it was wrong! ;-).
I think that version's still not right actually, the first line should
probably use filter-out not subst:
$(INSTALL) -m 644 $(filter-out include,$(wildcard mergetools/*)) \
'$(DESTDIR_SQ)$(mergetools_instdir_SQ)'
John
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] mergetools: Simplify how we handle "vim" and "defaults"
2013-01-26 12:12 ` John Keeping
2013-01-26 20:40 ` David Aguilar
@ 2013-01-27 4:57 ` Junio C Hamano
2013-01-27 5:07 ` David Aguilar
1 sibling, 1 reply; 8+ messages in thread
From: Junio C Hamano @ 2013-01-27 4:57 UTC (permalink / raw)
To: John Keeping; +Cc: David Aguilar, git
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.
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"
-}
^ permalink raw reply related [flat|nested] 8+ messages in thread* Re: [PATCH] mergetools: Simplify how we handle "vim" and "defaults"
2013-01-27 4:57 ` Junio C Hamano
@ 2013-01-27 5:07 ` David Aguilar
2013-01-27 5:35 ` David Aguilar
0 siblings, 1 reply; 8+ messages in thread
From: David Aguilar @ 2013-01-27 5:07 UTC (permalink / raw)
To: Junio C Hamano; +Cc: John Keeping, git
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.
> 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
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH] mergetools: Simplify how we handle "vim" and "defaults"
2013-01-27 5:07 ` David Aguilar
@ 2013-01-27 5:35 ` David Aguilar
2013-01-27 6:05 ` Junio C Hamano
0 siblings, 1 reply; 8+ messages in thread
From: David Aguilar @ 2013-01-27 5:35 UTC (permalink / raw)
To: Junio C Hamano; +Cc: John Keeping, git
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
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH] mergetools: Simplify how we handle "vim" and "defaults"
2013-01-27 5:35 ` David Aguilar
@ 2013-01-27 6:05 ` Junio C Hamano
0 siblings, 0 replies; 8+ messages in thread
From: Junio C Hamano @ 2013-01-27 6:05 UTC (permalink / raw)
To: David Aguilar; +Cc: John Keeping, git
David Aguilar <davvid@gmail.com> writes:
>> 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.
I was lazy and didn't want to think about rewriting your log
message, so I just queued it with this log message on top.
mergetools: remove mergetools/include/defaults.sh
This and its containing directory are used only to define trivial
fallback definition of five shell functions in a single script.
Defining them in-line at the location that wants to have them makes
the result much easier to read.
Signed-off-by: Junio C Hamano <gitster@pobox.com>
But as you said, removing the last line of your log message and
squashing the change into it would be more preferrable. Let me do
that later.
Thanks.
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2013-01-27 6:05 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-01-26 6:50 [PATCH] mergetools: Simplify how we handle "vim" and "defaults" David Aguilar
2013-01-26 12:12 ` John Keeping
2013-01-26 20:40 ` David Aguilar
2013-01-26 20:54 ` John Keeping
2013-01-27 4:57 ` Junio C Hamano
2013-01-27 5:07 ` David Aguilar
2013-01-27 5:35 ` David Aguilar
2013-01-27 6:05 ` Junio C Hamano
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).