* [PATCH 1/4] difftool--helper: Make style consistent with git
2011-08-18 7:23 [PATCH 0/4] Refactor git-mergetool--lib.sh David Aguilar
@ 2011-08-18 7:23 ` David Aguilar
2011-08-18 7:49 ` Sebastian Schuberth
2011-08-18 7:23 ` [PATCH 2/4] mergetool--lib: " David Aguilar
` (3 subsequent siblings)
4 siblings, 1 reply; 18+ messages in thread
From: David Aguilar @ 2011-08-18 7:23 UTC (permalink / raw)
To: Junio C Hamano
Cc: git, Jonathan Nieder, Tanguy Ortolo, Charles Bailey,
Sebastian Schuberth
Use the predominant conditional style where "then" appears
alone on the line after the test expression.
Signed-off-by: David Aguilar <davvid@gmail.com>
---
git-difftool--helper.sh | 18 ++++++++++++------
1 files changed, 12 insertions(+), 6 deletions(-)
diff --git a/git-difftool--helper.sh b/git-difftool--helper.sh
index 0594bf7..8452890 100755
--- a/git-difftool--helper.sh
+++ b/git-difftool--helper.sh
@@ -13,7 +13,8 @@ TOOL_MODE=diff
should_prompt () {
prompt_merge=$(git config --bool mergetool.prompt || echo true)
prompt=$(git config --bool difftool.prompt || echo $prompt_merge)
- if test "$prompt" = true; then
+ if test "$prompt" = true
+ then
test -z "$GIT_DIFFTOOL_NO_PROMPT"
else
test -n "$GIT_DIFFTOOL_PROMPT"
@@ -37,9 +38,11 @@ launch_merge_tool () {
# $LOCAL and $REMOTE are temporary files so prompt
# the user with the real $MERGED name before launching $merge_tool.
- if should_prompt; then
+ if should_prompt
+ then
printf "\nViewing: '$MERGED'\n"
- if use_ext_cmd; then
+ if use_ext_cmd
+ then
printf "Hit return to launch '%s': " \
"$GIT_DIFFTOOL_EXTCMD"
else
@@ -48,7 +51,8 @@ launch_merge_tool () {
read ans
fi
- if use_ext_cmd; then
+ if use_ext_cmd
+ then
export BASE
eval $GIT_DIFFTOOL_EXTCMD '"$LOCAL"' '"$REMOTE"'
else
@@ -56,8 +60,10 @@ launch_merge_tool () {
fi
}
-if ! use_ext_cmd; then
- if test -n "$GIT_DIFF_TOOL"; then
+if ! use_ext_cmd
+then
+ if test -n "$GIT_DIFF_TOOL"
+ then
merge_tool="$GIT_DIFF_TOOL"
else
merge_tool="$(get_merge_tool)" || exit
--
1.7.6.476.g57292
^ permalink raw reply related [flat|nested] 18+ messages in thread* Re: [PATCH 1/4] difftool--helper: Make style consistent with git
2011-08-18 7:23 ` [PATCH 1/4] difftool--helper: Make style consistent with git David Aguilar
@ 2011-08-18 7:49 ` Sebastian Schuberth
2011-08-18 9:08 ` David Aguilar
0 siblings, 1 reply; 18+ messages in thread
From: Sebastian Schuberth @ 2011-08-18 7:49 UTC (permalink / raw)
To: David Aguilar
Cc: Junio C Hamano, git, Jonathan Nieder, Tanguy Ortolo,
Charles Bailey
On Thu, Aug 18, 2011 at 09:23, David Aguilar <davvid@gmail.com> wrote:
> Use the predominant conditional style where "then" appears
> alone on the line after the test expression.
I support your effort to unify the style, but for my personal taste it
has gone the wrong way :-) Even if "then" on its own line was the
predominant style in the merge scripts, I would have voted for putting
all "then" in the same line as "if" as I find this easier to read and
comprehend. But then again it's obvious that such a change would
polarize ;-)
--
Sebastian Schuberth
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 1/4] difftool--helper: Make style consistent with git
2011-08-18 7:49 ` Sebastian Schuberth
@ 2011-08-18 9:08 ` David Aguilar
2011-08-18 17:31 ` Junio C Hamano
0 siblings, 1 reply; 18+ messages in thread
From: David Aguilar @ 2011-08-18 9:08 UTC (permalink / raw)
To: Sebastian Schuberth
Cc: Junio C Hamano, git, Jonathan Nieder, Tanguy Ortolo,
Charles Bailey
On Thu, Aug 18, 2011 at 09:49:33AM +0200, Sebastian Schuberth wrote:
> On Thu, Aug 18, 2011 at 09:23, David Aguilar <davvid@gmail.com> wrote:
>
> > Use the predominant conditional style where "then" appears
> > alone on the line after the test expression.
>
> I support your effort to unify the style, but for my personal taste it
> has gone the wrong way :-) Even if "then" on its own line was the
> predominant style in the merge scripts, I would have voted for putting
> all "then" in the same line as "if" as I find this easier to read and
> comprehend. But then again it's obvious that such a change would
> polarize ;-)
>
> --
> Sebastian Schuberth
I am fine reworking it either way.
There's not a specific note about it in
Documentation/CodingStyle.
If someone wants to make the final call I can patch the docs
and rework these patches as needed.
Thanks,
--
David
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 1/4] difftool--helper: Make style consistent with git
2011-08-18 9:08 ` David Aguilar
@ 2011-08-18 17:31 ` Junio C Hamano
0 siblings, 0 replies; 18+ messages in thread
From: Junio C Hamano @ 2011-08-18 17:31 UTC (permalink / raw)
To: David Aguilar
Cc: Sebastian Schuberth, git, Jonathan Nieder, Tanguy Ortolo,
Charles Bailey
David Aguilar <davvid@gmail.com> writes:
> I am fine reworking it either way.
> There's not a specific note about it in
> Documentation/CodingStyle.
>
> If someone wants to make the final call I can patch the docs
> and rework these patches as needed.
Please stick to (and document if not):
if test ...
then
action
elif condition that can become looong so more &&
condition needs to be split to next line &&
even more condition
then
action
else
action
fi
Thanks.
You can think of it as "war against semicolon" if you want.
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH 2/4] mergetool--lib: Make style consistent with git
2011-08-18 7:23 [PATCH 0/4] Refactor git-mergetool--lib.sh David Aguilar
2011-08-18 7:23 ` [PATCH 1/4] difftool--helper: Make style consistent with git David Aguilar
@ 2011-08-18 7:23 ` David Aguilar
2011-08-18 7:23 ` [PATCH 3/4] mergetool--lib: Refactor tools into separate files David Aguilar
` (2 subsequent siblings)
4 siblings, 0 replies; 18+ messages in thread
From: David Aguilar @ 2011-08-18 7:23 UTC (permalink / raw)
To: Junio C Hamano
Cc: git, Jonathan Nieder, Tanguy Ortolo, Charles Bailey,
Sebastian Schuberth
Use the predominant conditional style where "then" appears
alone on the line after the test expression.
Remove spaces after ">" output redirections.
Remove unnecessary parentheses around the kdiff3 commands.
Signed-off-by: David Aguilar <davvid@gmail.com>
---
git-mergetool--lib.sh | 153 ++++++++++++++++++++++++++++++++-----------------
1 files changed, 100 insertions(+), 53 deletions(-)
diff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh
index 91f90ac..03cfb19 100644
--- a/git-mergetool--lib.sh
+++ b/git-mergetool--lib.sh
@@ -32,10 +32,12 @@ translate_merge_tool_path () {
}
check_unchanged () {
- if test "$MERGED" -nt "$BACKUP"; then
+ if test "$MERGED" -nt "$BACKUP"
+ then
status=0
else
- while true; do
+ while true
+ do
echo "$MERGED seems unchanged."
printf "Was the merge successful? [y/n] "
read answer
@@ -53,17 +55,20 @@ valid_tool () {
kdiff3 | meld | opendiff | p4merge | tkdiff | vimdiff | vimdiff2 | xxdiff)
;; # happy
kompare)
- if ! diff_mode; then
+ if ! diff_mode
+ then
return 1
fi
;;
tortoisemerge)
- if ! merge_mode; then
+ if ! merge_mode
+ then
return 1
fi
;;
*)
- if test -z "$(get_merge_tool_cmd "$1")"; then
+ if test -z "$(get_merge_tool_cmd "$1")"
+ then
return 1
fi
;;
@@ -72,12 +77,14 @@ valid_tool () {
get_merge_tool_cmd () {
# Prints the custom command for a merge tool
- if test -n "$1"; then
+ if test -n "$1"
+ then
merge_tool="$1"
else
merge_tool="$(get_merge_tool)"
fi
- if diff_mode; then
+ if diff_mode
+ then
echo "$(git config difftool.$merge_tool.cmd ||
git config mergetool.$merge_tool.cmd)"
else
@@ -97,9 +104,11 @@ run_merge_tool () {
case "$1" in
araxis)
- if merge_mode; then
+ if merge_mode
+ then
touch "$BACKUP"
- if $base_present; then
+ if $base_present
+ then
"$merge_tool_path" -wait -merge -3 -a1 \
"$BASE" "$LOCAL" "$REMOTE" "$MERGED" \
>/dev/null 2>&1
@@ -115,9 +124,11 @@ run_merge_tool () {
fi
;;
bc3)
- if merge_mode; then
+ if merge_mode
+ then
touch "$BACKUP"
- if $base_present; then
+ if $base_present
+ then
"$merge_tool_path" "$LOCAL" "$REMOTE" "$BASE" \
-mergeoutput="$MERGED"
else
@@ -130,9 +141,11 @@ run_merge_tool () {
fi
;;
diffuse)
- if merge_mode; then
+ if merge_mode
+ then
touch "$BACKUP"
- if $base_present; then
+ if $base_present
+ then
"$merge_tool_path" \
"$LOCAL" "$MERGED" "$REMOTE" \
"$BASE" | cat
@@ -146,9 +159,11 @@ run_merge_tool () {
fi
;;
ecmerge)
- if merge_mode; then
+ if merge_mode
+ then
touch "$BACKUP"
- if $base_present; then
+ if $base_present
+ then
"$merge_tool_path" "$BASE" "$LOCAL" "$REMOTE" \
--default --mode=merge3 --to="$MERGED"
else
@@ -162,8 +177,10 @@ run_merge_tool () {
fi
;;
emerge)
- if merge_mode; then
- if $base_present; then
+ if merge_mode
+ then
+ if $base_present
+ then
"$merge_tool_path" \
-f emerge-files-with-ancestor-command \
"$LOCAL" "$REMOTE" "$BASE" \
@@ -181,9 +198,11 @@ run_merge_tool () {
fi
;;
gvimdiff|vimdiff)
- if merge_mode; then
+ if merge_mode
+ then
touch "$BACKUP"
- if $base_present; then
+ if $base_present
+ then
"$merge_tool_path" -f -d -c "wincmd J" \
"$MERGED" "$LOCAL" "$BASE" "$REMOTE"
else
@@ -198,7 +217,8 @@ run_merge_tool () {
fi
;;
gvimdiff2|vimdiff2)
- if merge_mode; then
+ if merge_mode
+ then
touch "$BACKUP"
"$merge_tool_path" -f -d -c "wincmd l" \
"$LOCAL" "$MERGED" "$REMOTE"
@@ -210,36 +230,39 @@ run_merge_tool () {
fi
;;
kdiff3)
- if merge_mode; then
- if $base_present; then
- ("$merge_tool_path" --auto \
+ if merge_mode
+ then
+ if $base_present
+ then
+ "$merge_tool_path" --auto \
--L1 "$MERGED (Base)" \
--L2 "$MERGED (Local)" \
--L3 "$MERGED (Remote)" \
-o "$MERGED" \
"$BASE" "$LOCAL" "$REMOTE" \
- > /dev/null 2>&1)
+ >/dev/null 2>&1
else
- ("$merge_tool_path" --auto \
+ "$merge_tool_path" --auto \
--L1 "$MERGED (Local)" \
--L2 "$MERGED (Remote)" \
-o "$MERGED" \
"$LOCAL" "$REMOTE" \
- > /dev/null 2>&1)
+ >/dev/null 2>&1
fi
status=$?
else
- ("$merge_tool_path" --auto \
+ "$merge_tool_path" --auto \
--L1 "$MERGED (A)" \
--L2 "$MERGED (B)" "$LOCAL" "$REMOTE" \
- > /dev/null 2>&1)
+ >/dev/null 2>&1
fi
;;
kompare)
"$merge_tool_path" "$LOCAL" "$REMOTE"
;;
meld)
- if merge_mode; then
+ if merge_mode
+ then
touch "$BACKUP"
"$merge_tool_path" "$LOCAL" "$MERGED" "$REMOTE"
check_unchanged
@@ -248,9 +271,11 @@ run_merge_tool () {
fi
;;
opendiff)
- if merge_mode; then
+ if merge_mode
+ then
touch "$BACKUP"
- if $base_present; then
+ if $base_present
+ then
"$merge_tool_path" "$LOCAL" "$REMOTE" \
-ancestor "$BASE" \
-merge "$MERGED" | cat
@@ -264,7 +289,8 @@ run_merge_tool () {
fi
;;
p4merge)
- if merge_mode; then
+ if merge_mode
+ then
touch "$BACKUP"
$base_present || >"$BASE"
"$merge_tool_path" "$BASE" "$LOCAL" "$REMOTE" "$MERGED"
@@ -274,8 +300,10 @@ run_merge_tool () {
fi
;;
tkdiff)
- if merge_mode; then
- if $base_present; then
+ if merge_mode
+ then
+ if $base_present
+ then
"$merge_tool_path" -a "$BASE" \
-o "$MERGED" "$LOCAL" "$REMOTE"
else
@@ -288,7 +316,8 @@ run_merge_tool () {
fi
;;
tortoisemerge)
- if $base_present; then
+ if $base_present
+ then
touch "$BACKUP"
"$merge_tool_path" \
-base:"$BASE" -mine:"$LOCAL" \
@@ -300,9 +329,11 @@ run_merge_tool () {
fi
;;
xxdiff)
- if merge_mode; then
+ if merge_mode
+ then
touch "$BACKUP"
- if $base_present; then
+ if $base_present
+ then
"$merge_tool_path" -X --show-merged-pane \
-R 'Accel.SaveAsMerged: "Ctrl-S"' \
-R 'Accel.Search: "Ctrl+F"' \
@@ -327,16 +358,20 @@ run_merge_tool () {
;;
*)
merge_tool_cmd="$(get_merge_tool_cmd "$1")"
- if test -z "$merge_tool_cmd"; then
- if merge_mode; then
+ if test -z "$merge_tool_cmd"
+ then
+ if merge_mode
+ then
status=1
fi
break
fi
- if merge_mode; then
+ if merge_mode
+ then
trust_exit_code="$(git config --bool \
mergetool."$1".trustExitCode || echo false)"
- if test "$trust_exit_code" = "false"; then
+ if test "$trust_exit_code" = "false"
+ then
touch "$BACKUP"
( eval $merge_tool_cmd )
check_unchanged
@@ -353,13 +388,16 @@ run_merge_tool () {
}
guess_merge_tool () {
- if merge_mode; then
+ if merge_mode
+ then
tools="tortoisemerge"
else
tools="kompare"
fi
- if test -n "$DISPLAY"; then
- if test -n "$GNOME_DESKTOP_SESSION_ID" ; then
+ if test -n "$DISPLAY"
+ then
+ if test -n "$GNOME_DESKTOP_SESSION_ID"
+ then
tools="meld opendiff kdiff3 tkdiff xxdiff $tools"
else
tools="opendiff kdiff3 tkdiff xxdiff meld $tools"
@@ -380,7 +418,8 @@ guess_merge_tool () {
for i in $tools
do
merge_tool_path="$(translate_merge_tool_path "$i")"
- if type "$merge_tool_path" > /dev/null 2>&1; then
+ if type "$merge_tool_path" >/dev/null 2>&1
+ then
echo "$i"
return 0
fi
@@ -393,12 +432,14 @@ guess_merge_tool () {
get_configured_merge_tool () {
# Diff mode first tries diff.tool and falls back to merge.tool.
# Merge mode only checks merge.tool
- if diff_mode; then
+ if diff_mode
+ then
merge_tool=$(git config diff.tool || git config merge.tool)
else
merge_tool=$(git config merge.tool)
fi
- if test -n "$merge_tool" && ! valid_tool "$merge_tool"; then
+ if test -n "$merge_tool" && ! valid_tool "$merge_tool"
+ then
echo >&2 "git config option $TOOL_MODE.tool set to unknown tool: $merge_tool"
echo >&2 "Resetting to default..."
return 1
@@ -408,26 +449,31 @@ get_configured_merge_tool () {
get_merge_tool_path () {
# A merge tool has been set, so verify that it's valid.
- if test -n "$1"; then
+ if test -n "$1"
+ then
merge_tool="$1"
else
merge_tool="$(get_merge_tool)"
fi
- if ! valid_tool "$merge_tool"; then
+ if ! valid_tool "$merge_tool"
+ then
echo >&2 "Unknown merge tool $merge_tool"
exit 1
fi
- if diff_mode; then
+ if diff_mode
+ then
merge_tool_path=$(git config difftool."$merge_tool".path ||
git config mergetool."$merge_tool".path)
else
merge_tool_path=$(git config mergetool."$merge_tool".path)
fi
- if test -z "$merge_tool_path"; then
+ if test -z "$merge_tool_path"
+ then
merge_tool_path="$(translate_merge_tool_path "$merge_tool")"
fi
if test -z "$(get_merge_tool_cmd "$merge_tool")" &&
- ! type "$merge_tool_path" > /dev/null 2>&1; then
+ ! type "$merge_tool_path" >/dev/null 2>&1
+ then
echo >&2 "The $TOOL_MODE tool $merge_tool is not available as"\
"'$merge_tool_path'"
exit 1
@@ -439,7 +485,8 @@ get_merge_tool () {
# Check if a merge tool has been configured
merge_tool=$(get_configured_merge_tool)
# Try to guess an appropriate merge tool if no tool has been set.
- if test -z "$merge_tool"; then
+ if test -z "$merge_tool"
+ then
merge_tool="$(guess_merge_tool)" || exit
fi
echo "$merge_tool"
--
1.7.6.476.g57292
^ permalink raw reply related [flat|nested] 18+ messages in thread* [PATCH 3/4] mergetool--lib: Refactor tools into separate files
2011-08-18 7:23 [PATCH 0/4] Refactor git-mergetool--lib.sh David Aguilar
2011-08-18 7:23 ` [PATCH 1/4] difftool--helper: Make style consistent with git David Aguilar
2011-08-18 7:23 ` [PATCH 2/4] mergetool--lib: " David Aguilar
@ 2011-08-18 7:23 ` David Aguilar
2011-10-09 9:17 ` [PATCH da/difftool-mergtool-refactor] Makefile: fix permissions of mergetools/ checked out with permissive umask Jonathan Nieder
2011-08-18 7:23 ` [PATCH 4/4] mergetools/meld: Use '--output' when available David Aguilar
2011-08-18 7:57 ` [PATCH 0/4] Refactor git-mergetool--lib.sh Sebastian Schuberth
4 siblings, 1 reply; 18+ messages in thread
From: David Aguilar @ 2011-08-18 7:23 UTC (permalink / raw)
To: Junio C Hamano
Cc: git, Jonathan Nieder, Tanguy Ortolo, Charles Bailey,
Sebastian Schuberth
Individual merge tools are now defined in a mergetools/$tool
file which is sourced at runtime.
The individual files are installed into $(git --exec-path)/mergetools/.
New tools can be added by creating a new file instead of editing the
git-mergetool--lib.sh scriptlet.
http://thread.gmane.org/gmane.comp.version-control.git/134906/focus=135006
Signed-off-by: David Aguilar <davvid@gmail.com>
---
Makefile | 11 ++
git-mergetool--lib.sh | 385 ++++++---------------------------------------
mergetools/araxis | 21 +++
mergetools/bc3 | 20 +++
mergetools/defaults | 46 ++++++
mergetools/diffuse | 17 ++
mergetools/ecmerge | 16 ++
mergetools/emerge | 23 +++
mergetools/kdiff3 | 24 +++
mergetools/kompare | 7 +
mergetools/meld | 9 +
mergetools/opendiff | 16 ++
mergetools/p4merge | 10 ++
mergetools/tkdiff | 12 ++
mergetools/tortoisemerge | 17 ++
mergetools/vim | 44 ++++++
mergetools/xxdiff | 25 +++
17 files changed, 370 insertions(+), 333 deletions(-)
create mode 100644 mergetools/araxis
create mode 100644 mergetools/bc3
create mode 100644 mergetools/defaults
create mode 100644 mergetools/diffuse
create mode 100644 mergetools/ecmerge
create mode 100644 mergetools/emerge
create mode 100644 mergetools/kdiff3
create mode 100644 mergetools/kompare
create mode 100644 mergetools/meld
create mode 100644 mergetools/opendiff
create mode 100644 mergetools/p4merge
create mode 100644 mergetools/tkdiff
create mode 100644 mergetools/tortoisemerge
create mode 100644 mergetools/vim
create mode 100644 mergetools/xxdiff
diff --git a/Makefile b/Makefile
index 8dd782f..6a68e3a 100644
--- a/Makefile
+++ b/Makefile
@@ -302,6 +302,7 @@ bindir = $(prefix)/$(bindir_relative)
mandir = share/man
infodir = share/info
gitexecdir = libexec/git-core
+mergetoolsdir = $(gitexecdir)/mergetools
sharedir = $(prefix)/share
gitwebdir = $(sharedir)/gitweb
template_dir = share/git-core/templates
@@ -2257,6 +2258,13 @@ endif
gitexec_instdir_SQ = $(subst ','\'',$(gitexec_instdir))
export gitexec_instdir
+ifneq ($(filter /%,$(firstword $(mergetoolsdir))),)
+mergetools_instdir = $(mergetoolsdir)
+else
+mergetools_instdir = $(prefix)/$(mergetoolsdir)
+endif
+mergetools_instdir_SQ = $(subst ','\'',$(mergetools_instdir))
+
install_bindir_programs := $(patsubst %,%$X,$(BINDIR_PROGRAMS_NEED_X)) $(BINDIR_PROGRAMS_NO_X)
install: all
@@ -2266,6 +2274,9 @@ install: all
$(INSTALL) -m 644 $(SCRIPT_LIB) '$(DESTDIR_SQ)$(gitexec_instdir_SQ)'
$(INSTALL) $(install_bindir_programs) '$(DESTDIR_SQ)$(bindir_SQ)'
$(MAKE) -C templates DESTDIR='$(DESTDIR_SQ)' install
+ $(INSTALL) -d -m 755 '$(DESTDIR_SQ)$(mergetools_instdir_SQ)'
+ (cd mergetools && $(TAR) cf - .) | \
+ (cd '$(DESTDIR_SQ)$(mergetools_instdir_SQ)' && umask 022 && $(TAR) xof -)
ifndef NO_PERL
$(MAKE) -C perl prefix='$(prefix_SQ)' DESTDIR='$(DESTDIR_SQ)' install
$(MAKE) -C gitweb install
diff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh
index 03cfb19..2421700 100644
--- a/git-mergetool--lib.sh
+++ b/git-mergetool--lib.sh
@@ -9,26 +9,7 @@ merge_mode() {
}
translate_merge_tool_path () {
- case "$1" in
- araxis)
- echo compare
- ;;
- bc3)
- echo bcompare
- ;;
- emerge)
- echo emacs
- ;;
- gvimdiff|gvimdiff2)
- echo gvim
- ;;
- vimdiff|vimdiff2)
- echo vim
- ;;
- *)
- echo "$1"
- ;;
- esac
+ echo "$1"
}
check_unchanged () {
@@ -49,40 +30,55 @@ check_unchanged () {
fi
}
+valid_tool_config () {
+ if test -n "$(get_merge_tool_cmd "$1")"
+ then
+ return 0
+ else
+ return 1
+ fi
+}
+
valid_tool () {
+ setup_tool "$1" || valid_tool_config "$1"
+}
+
+setup_tool () {
case "$1" in
- araxis | bc3 | diffuse | ecmerge | emerge | gvimdiff | gvimdiff2 | \
- kdiff3 | meld | opendiff | p4merge | tkdiff | vimdiff | vimdiff2 | xxdiff)
- ;; # happy
- kompare)
- if ! diff_mode
- then
- return 1
- fi
- ;;
- tortoisemerge)
- if ! merge_mode
- then
- return 1
- fi
+ vim*|gvim*)
+ tool=vim
;;
*)
- if test -z "$(get_merge_tool_cmd "$1")"
- then
- return 1
- fi
+ tool="$1"
;;
esac
+ mergetools="$(git --exec-path)/mergetools"
+
+ # Load the default definitions
+ . "$mergetools/defaults"
+ if ! test -f "$mergetools/$tool"
+ then
+ return 1
+ fi
+
+ # Load the redefined functions
+ . "$mergetools/$tool"
+
+ if merge_mode && ! can_merge
+ then
+ echo "error: '$tool' can not be used to resolve merges" >&2
+ exit 1
+ elif diff_mode && ! can_diff
+ then
+ echo "error: '$tool' can only be used to resolve merges" >&2
+ exit 1
+ fi
+ return 0
}
get_merge_tool_cmd () {
# Prints the custom command for a merge tool
- if test -n "$1"
- then
- merge_tool="$1"
- else
- merge_tool="$(get_merge_tool)"
- fi
+ merge_tool="$1"
if diff_mode
then
echo "$(git config difftool.$merge_tool.cmd ||
@@ -92,6 +88,7 @@ get_merge_tool_cmd () {
fi
}
+# Entry point for running tools
run_merge_tool () {
# If GIT_PREFIX is empty then we cannot use it in tools
# that expect to be able to chdir() to its value.
@@ -102,288 +99,15 @@ run_merge_tool () {
base_present="$2"
status=0
- case "$1" in
- araxis)
- if merge_mode
- then
- touch "$BACKUP"
- if $base_present
- then
- "$merge_tool_path" -wait -merge -3 -a1 \
- "$BASE" "$LOCAL" "$REMOTE" "$MERGED" \
- >/dev/null 2>&1
- else
- "$merge_tool_path" -wait -2 \
- "$LOCAL" "$REMOTE" "$MERGED" \
- >/dev/null 2>&1
- fi
- check_unchanged
- else
- "$merge_tool_path" -wait -2 "$LOCAL" "$REMOTE" \
- >/dev/null 2>&1
- fi
- ;;
- bc3)
- if merge_mode
- then
- touch "$BACKUP"
- if $base_present
- then
- "$merge_tool_path" "$LOCAL" "$REMOTE" "$BASE" \
- -mergeoutput="$MERGED"
- else
- "$merge_tool_path" "$LOCAL" "$REMOTE" \
- -mergeoutput="$MERGED"
- fi
- check_unchanged
- else
- "$merge_tool_path" "$LOCAL" "$REMOTE"
- fi
- ;;
- diffuse)
- if merge_mode
- then
- touch "$BACKUP"
- if $base_present
- then
- "$merge_tool_path" \
- "$LOCAL" "$MERGED" "$REMOTE" \
- "$BASE" | cat
- else
- "$merge_tool_path" \
- "$LOCAL" "$MERGED" "$REMOTE" | cat
- fi
- check_unchanged
- else
- "$merge_tool_path" "$LOCAL" "$REMOTE" | cat
- fi
- ;;
- ecmerge)
- if merge_mode
- then
- touch "$BACKUP"
- if $base_present
- then
- "$merge_tool_path" "$BASE" "$LOCAL" "$REMOTE" \
- --default --mode=merge3 --to="$MERGED"
- else
- "$merge_tool_path" "$LOCAL" "$REMOTE" \
- --default --mode=merge2 --to="$MERGED"
- fi
- check_unchanged
- else
- "$merge_tool_path" --default --mode=diff2 \
- "$LOCAL" "$REMOTE"
- fi
- ;;
- emerge)
- if merge_mode
- then
- if $base_present
- then
- "$merge_tool_path" \
- -f emerge-files-with-ancestor-command \
- "$LOCAL" "$REMOTE" "$BASE" \
- "$(basename "$MERGED")"
- else
- "$merge_tool_path" \
- -f emerge-files-command \
- "$LOCAL" "$REMOTE" \
- "$(basename "$MERGED")"
- fi
- status=$?
- else
- "$merge_tool_path" -f emerge-files-command \
- "$LOCAL" "$REMOTE"
- fi
- ;;
- gvimdiff|vimdiff)
- if merge_mode
- then
- touch "$BACKUP"
- if $base_present
- then
- "$merge_tool_path" -f -d -c "wincmd J" \
- "$MERGED" "$LOCAL" "$BASE" "$REMOTE"
- else
- "$merge_tool_path" -f -d -c "wincmd l" \
- "$LOCAL" "$MERGED" "$REMOTE"
- fi
- check_unchanged
- else
- "$merge_tool_path" -R -f -d -c "wincmd l" \
- -c 'cd $GIT_PREFIX' \
- "$LOCAL" "$REMOTE"
- fi
- ;;
- gvimdiff2|vimdiff2)
- if merge_mode
- then
- touch "$BACKUP"
- "$merge_tool_path" -f -d -c "wincmd l" \
- "$LOCAL" "$MERGED" "$REMOTE"
- check_unchanged
- else
- "$merge_tool_path" -R -f -d -c "wincmd l" \
- -c 'cd $GIT_PREFIX' \
- "$LOCAL" "$REMOTE"
- fi
- ;;
- kdiff3)
- if merge_mode
- then
- if $base_present
- then
- "$merge_tool_path" --auto \
- --L1 "$MERGED (Base)" \
- --L2 "$MERGED (Local)" \
- --L3 "$MERGED (Remote)" \
- -o "$MERGED" \
- "$BASE" "$LOCAL" "$REMOTE" \
- >/dev/null 2>&1
- else
- "$merge_tool_path" --auto \
- --L1 "$MERGED (Local)" \
- --L2 "$MERGED (Remote)" \
- -o "$MERGED" \
- "$LOCAL" "$REMOTE" \
- >/dev/null 2>&1
- fi
- status=$?
- else
- "$merge_tool_path" --auto \
- --L1 "$MERGED (A)" \
- --L2 "$MERGED (B)" "$LOCAL" "$REMOTE" \
- >/dev/null 2>&1
- fi
- ;;
- kompare)
- "$merge_tool_path" "$LOCAL" "$REMOTE"
- ;;
- meld)
- if merge_mode
- then
- touch "$BACKUP"
- "$merge_tool_path" "$LOCAL" "$MERGED" "$REMOTE"
- check_unchanged
- else
- "$merge_tool_path" "$LOCAL" "$REMOTE"
- fi
- ;;
- opendiff)
- if merge_mode
- then
- touch "$BACKUP"
- if $base_present
- then
- "$merge_tool_path" "$LOCAL" "$REMOTE" \
- -ancestor "$BASE" \
- -merge "$MERGED" | cat
- else
- "$merge_tool_path" "$LOCAL" "$REMOTE" \
- -merge "$MERGED" | cat
- fi
- check_unchanged
- else
- "$merge_tool_path" "$LOCAL" "$REMOTE" | cat
- fi
- ;;
- p4merge)
- if merge_mode
- then
- touch "$BACKUP"
- $base_present || >"$BASE"
- "$merge_tool_path" "$BASE" "$LOCAL" "$REMOTE" "$MERGED"
- check_unchanged
- else
- "$merge_tool_path" "$LOCAL" "$REMOTE"
- fi
- ;;
- tkdiff)
- if merge_mode
- then
- if $base_present
- then
- "$merge_tool_path" -a "$BASE" \
- -o "$MERGED" "$LOCAL" "$REMOTE"
- else
- "$merge_tool_path" \
- -o "$MERGED" "$LOCAL" "$REMOTE"
- fi
- status=$?
- else
- "$merge_tool_path" "$LOCAL" "$REMOTE"
- fi
- ;;
- tortoisemerge)
- if $base_present
- then
- touch "$BACKUP"
- "$merge_tool_path" \
- -base:"$BASE" -mine:"$LOCAL" \
- -theirs:"$REMOTE" -merged:"$MERGED"
- check_unchanged
- else
- echo "TortoiseMerge cannot be used without a base" 1>&2
- status=1
- fi
- ;;
- xxdiff)
- if merge_mode
- then
- touch "$BACKUP"
- if $base_present
- then
- "$merge_tool_path" -X --show-merged-pane \
- -R 'Accel.SaveAsMerged: "Ctrl-S"' \
- -R 'Accel.Search: "Ctrl+F"' \
- -R 'Accel.SearchForward: "Ctrl-G"' \
- --merged-file "$MERGED" \
- "$LOCAL" "$BASE" "$REMOTE"
- else
- "$merge_tool_path" -X $extra \
- -R 'Accel.SaveAsMerged: "Ctrl-S"' \
- -R 'Accel.Search: "Ctrl+F"' \
- -R 'Accel.SearchForward: "Ctrl-G"' \
- --merged-file "$MERGED" \
- "$LOCAL" "$REMOTE"
- fi
- check_unchanged
- else
- "$merge_tool_path" \
- -R 'Accel.Search: "Ctrl+F"' \
- -R 'Accel.SearchForward: "Ctrl-G"' \
- "$LOCAL" "$REMOTE"
- fi
- ;;
- *)
- merge_tool_cmd="$(get_merge_tool_cmd "$1")"
- if test -z "$merge_tool_cmd"
- then
- if merge_mode
- then
- status=1
- fi
- break
- fi
- if merge_mode
- then
- trust_exit_code="$(git config --bool \
- mergetool."$1".trustExitCode || echo false)"
- if test "$trust_exit_code" = "false"
- then
- touch "$BACKUP"
- ( eval $merge_tool_cmd )
- check_unchanged
- else
- ( eval $merge_tool_cmd )
- status=$?
- fi
- else
- ( eval $merge_tool_cmd )
- fi
- ;;
- esac
+ # Bring tool-specific functions into scope
+ setup_tool "$1"
+
+ if merge_mode
+ then
+ merge_cmd "$1"
+ else
+ diff_cmd "$1"
+ fi
return $status
}
@@ -449,12 +173,7 @@ get_configured_merge_tool () {
get_merge_tool_path () {
# A merge tool has been set, so verify that it's valid.
- if test -n "$1"
- then
- merge_tool="$1"
- else
- merge_tool="$(get_merge_tool)"
- fi
+ merge_tool="$1"
if ! valid_tool "$merge_tool"
then
echo >&2 "Unknown merge tool $merge_tool"
@@ -483,7 +202,7 @@ get_merge_tool_path () {
get_merge_tool () {
# Check if a merge tool has been configured
- merge_tool=$(get_configured_merge_tool)
+ merge_tool="$(get_configured_merge_tool)"
# Try to guess an appropriate merge tool if no tool has been set.
if test -z "$merge_tool"
then
diff --git a/mergetools/araxis b/mergetools/araxis
new file mode 100644
index 0000000..69a9c8c
--- /dev/null
+++ b/mergetools/araxis
@@ -0,0 +1,21 @@
+diff_cmd () {
+ "$merge_tool_path" -wait -2 "$LOCAL" "$REMOTE" >/dev/null 2>&1
+}
+
+merge_cmd () {
+ touch "$BACKUP"
+ if $base_present
+ then
+ "$merge_tool_path" -wait -merge -3 -a1 \
+ "$BASE" "$LOCAL" "$REMOTE" "$MERGED" >/dev/null 2>&1
+ else
+ "$merge_tool_path" -wait -2 \
+ "$LOCAL" "$REMOTE" "$MERGED" >/dev/null 2>&1
+ fi
+ check_unchanged
+}
+
+translate_merge_tool_path() {
+ echo compare
+}
+
diff --git a/mergetools/bc3 b/mergetools/bc3
new file mode 100644
index 0000000..27b3dd4
--- /dev/null
+++ b/mergetools/bc3
@@ -0,0 +1,20 @@
+diff_cmd () {
+ "$merge_tool_path" "$LOCAL" "$REMOTE"
+}
+
+merge_cmd () {
+ touch "$BACKUP"
+ if $base_present
+ then
+ "$merge_tool_path" "$LOCAL" "$REMOTE" "$BASE" \
+ -mergeoutput="$MERGED"
+ else
+ "$merge_tool_path" "$LOCAL" "$REMOTE" \
+ -mergeoutput="$MERGED"
+ fi
+ check_unchanged
+}
+
+translate_merge_tool_path() {
+ echo bcompare
+}
diff --git a/mergetools/defaults b/mergetools/defaults
new file mode 100644
index 0000000..1d8f2a3
--- /dev/null
+++ b/mergetools/defaults
@@ -0,0 +1,46 @@
+# Redefined by builtin tools
+can_merge () {
+ return 0
+}
+
+can_diff () {
+ return 0
+}
+
+diff_cmd () {
+ merge_tool_cmd="$(get_merge_tool_cmd "$1")"
+ if test -z "$merge_tool_cmd"
+ then
+ status=1
+ break
+ fi
+ ( eval $merge_tool_cmd )
+ status=$?
+ return $status
+}
+
+merge_cmd () {
+ merge_tool_cmd="$(get_merge_tool_cmd "$1")"
+ if test -z "$merge_tool_cmd"
+ then
+ status=1
+ break
+ fi
+ trust_exit_code="$(git config --bool \
+ mergetool."$1".trustExitCode || echo false)"
+ if test "$trust_exit_code" = "false"
+ then
+ touch "$BACKUP"
+ ( eval $merge_tool_cmd )
+ status=$?
+ check_unchanged
+ else
+ ( eval $merge_tool_cmd )
+ status=$?
+ fi
+ return $status
+}
+
+translate_merge_tool_path () {
+ echo "$1"
+}
diff --git a/mergetools/diffuse b/mergetools/diffuse
new file mode 100644
index 0000000..02e0843
--- /dev/null
+++ b/mergetools/diffuse
@@ -0,0 +1,17 @@
+diff_cmd () {
+ "$merge_tool_path" "$LOCAL" "$REMOTE" | cat
+}
+
+merge_cmd () {
+ touch "$BACKUP"
+ if $base_present
+ then
+ "$merge_tool_path" \
+ "$LOCAL" "$MERGED" "$REMOTE" \
+ "$BASE" | cat
+ else
+ "$merge_tool_path" \
+ "$LOCAL" "$MERGED" "$REMOTE" | cat
+ fi
+ check_unchanged
+}
diff --git a/mergetools/ecmerge b/mergetools/ecmerge
new file mode 100644
index 0000000..13c2e43
--- /dev/null
+++ b/mergetools/ecmerge
@@ -0,0 +1,16 @@
+diff_cmd () {
+ "$merge_tool_path" --default --mode=diff2 "$LOCAL" "$REMOTE"
+}
+
+merge_cmd () {
+ touch "$BACKUP"
+ if $base_present
+ then
+ "$merge_tool_path" "$BASE" "$LOCAL" "$REMOTE" \
+ --default --mode=merge3 --to="$MERGED"
+ else
+ "$merge_tool_path" "$LOCAL" "$REMOTE" \
+ --default --mode=merge2 --to="$MERGED"
+ fi
+ check_unchanged
+}
diff --git a/mergetools/emerge b/mergetools/emerge
new file mode 100644
index 0000000..f96d9e5
--- /dev/null
+++ b/mergetools/emerge
@@ -0,0 +1,23 @@
+diff_cmd () {
+ "$merge_tool_path" -f emerge-files-command "$LOCAL" "$REMOTE"
+}
+
+merge_cmd () {
+ if $base_present
+ then
+ "$merge_tool_path" \
+ -f emerge-files-with-ancestor-command \
+ "$LOCAL" "$REMOTE" "$BASE" \
+ "$(basename "$MERGED")"
+ else
+ "$merge_tool_path" \
+ -f emerge-files-command \
+ "$LOCAL" "$REMOTE" \
+ "$(basename "$MERGED")"
+ fi
+ status=$?
+}
+
+translate_merge_tool_path() {
+ echo emacs
+}
diff --git a/mergetools/kdiff3 b/mergetools/kdiff3
new file mode 100644
index 0000000..28fead4
--- /dev/null
+++ b/mergetools/kdiff3
@@ -0,0 +1,24 @@
+diff_cmd () {
+ "$merge_tool_path" --auto \
+ --L1 "$MERGED (A)" --L2 "$MERGED (B)" \
+ "$LOCAL" "$REMOTE" >/dev/null 2>&1
+}
+
+merge_cmd () {
+ if $base_present
+ then
+ "$merge_tool_path" --auto \
+ --L1 "$MERGED (Base)" \
+ --L2 "$MERGED (Local)" \
+ --L3 "$MERGED (Remote)" \
+ -o "$MERGED" "$BASE" "$LOCAL" "$REMOTE" \
+ >/dev/null 2>&1
+ else
+ "$merge_tool_path" --auto \
+ --L1 "$MERGED (Local)" \
+ --L2 "$MERGED (Remote)" \
+ -o "$MERGED" "$LOCAL" "$REMOTE" \
+ >/dev/null 2>&1
+ fi
+ status=$?
+}
diff --git a/mergetools/kompare b/mergetools/kompare
new file mode 100644
index 0000000..433686c
--- /dev/null
+++ b/mergetools/kompare
@@ -0,0 +1,7 @@
+can_merge () {
+ return 1
+}
+
+diff_cmd () {
+ "$merge_tool_path" "$LOCAL" "$REMOTE"
+}
diff --git a/mergetools/meld b/mergetools/meld
new file mode 100644
index 0000000..73d70ae
--- /dev/null
+++ b/mergetools/meld
@@ -0,0 +1,9 @@
+diff_cmd () {
+ "$merge_tool_path" "$LOCAL" "$REMOTE"
+}
+
+merge_cmd () {
+ touch "$BACKUP"
+ "$merge_tool_path" "$LOCAL" "$MERGED" "$REMOTE"
+ check_unchanged
+}
diff --git a/mergetools/opendiff b/mergetools/opendiff
new file mode 100644
index 0000000..0942b2a
--- /dev/null
+++ b/mergetools/opendiff
@@ -0,0 +1,16 @@
+diff_cmd () {
+ "$merge_tool_path" "$LOCAL" "$REMOTE" | cat
+}
+
+merge_cmd () {
+ touch "$BACKUP"
+ if $base_present
+ then
+ "$merge_tool_path" "$LOCAL" "$REMOTE" \
+ -ancestor "$BASE" -merge "$MERGED" | cat
+ else
+ "$merge_tool_path" "$LOCAL" "$REMOTE" \
+ -merge "$MERGED" | cat
+ fi
+ check_unchanged
+}
diff --git a/mergetools/p4merge b/mergetools/p4merge
new file mode 100644
index 0000000..1a45c1b
--- /dev/null
+++ b/mergetools/p4merge
@@ -0,0 +1,10 @@
+diff_cmd () {
+ "$merge_tool_path" "$LOCAL" "$REMOTE"
+}
+
+merge_cmd () {
+ touch "$BACKUP"
+ $base_present || >"$BASE"
+ "$merge_tool_path" "$BASE" "$LOCAL" "$REMOTE" "$MERGED"
+ check_unchanged
+}
diff --git a/mergetools/tkdiff b/mergetools/tkdiff
new file mode 100644
index 0000000..618c438
--- /dev/null
+++ b/mergetools/tkdiff
@@ -0,0 +1,12 @@
+diff_cmd () {
+ "$merge_tool_path" "$LOCAL" "$REMOTE"
+}
+
+merge_cmd () {
+ if $base_present
+ then
+ "$merge_tool_path" -a "$BASE" -o "$MERGED" "$LOCAL" "$REMOTE"
+ else
+ "$merge_tool_path" -o "$MERGED" "$LOCAL" "$REMOTE"
+ fi
+}
diff --git a/mergetools/tortoisemerge b/mergetools/tortoisemerge
new file mode 100644
index 0000000..ed7db49
--- /dev/null
+++ b/mergetools/tortoisemerge
@@ -0,0 +1,17 @@
+can_diff () {
+ return 1
+}
+
+merge_cmd () {
+ if $base_present
+ then
+ touch "$BACKUP"
+ "$merge_tool_path" \
+ -base:"$BASE" -mine:"$LOCAL" \
+ -theirs:"$REMOTE" -merged:"$MERGED"
+ check_unchanged
+ else
+ echo "TortoiseMerge cannot be used without a base" 1>&2
+ return 1
+ fi
+}
diff --git a/mergetools/vim b/mergetools/vim
new file mode 100644
index 0000000..619594a
--- /dev/null
+++ b/mergetools/vim
@@ -0,0 +1,44 @@
+diff_cmd () {
+ case "$1" in
+ gvimdiff|vimdiff)
+ "$merge_tool_path" -R -f -d \
+ -c 'wincmd l' -c 'cd $GIT_PREFIX' "$LOCAL" "$REMOTE"
+ ;;
+ gvimdiff2|vimdiff2)
+ "$merge_tool_path" -R -f -d \
+ -c 'wincmd l' -c 'cd $GIT_PREFIX' "$LOCAL" "$REMOTE"
+ ;;
+ esac
+}
+
+merge_cmd () {
+ touch "$BACKUP"
+ case "$1" in
+ gvimdiff|vimdiff)
+ if $base_present
+ then
+ "$merge_tool_path" -f -d -c 'wincmd J' \
+ "$MERGED" "$LOCAL" "$BASE" "$REMOTE"
+ else
+ "$merge_tool_path" -f -d -c 'wincmd l' \
+ "$LOCAL" "$MERGED" "$REMOTE"
+ fi
+ ;;
+ gvimdiff2|vimdiff2)
+ "$merge_tool_path" -f -d -c 'wincmd l' \
+ "$LOCAL" "$MERGED" "$REMOTE"
+ ;;
+ esac
+ check_unchanged
+}
+
+translate_merge_tool_path() {
+ case "$1" in
+ gvimdiff|gvimdiff2)
+ echo gvim
+ ;;
+ vimdiff|vimdiff2)
+ echo vim
+ ;;
+ esac
+}
diff --git a/mergetools/xxdiff b/mergetools/xxdiff
new file mode 100644
index 0000000..05b4433
--- /dev/null
+++ b/mergetools/xxdiff
@@ -0,0 +1,25 @@
+diff_cmd () {
+ "$merge_tool_path" \
+ -R 'Accel.Search: "Ctrl+F"' \
+ -R 'Accel.SearchForward: "Ctrl-G"' \
+ "$LOCAL" "$REMOTE"
+}
+
+merge_cmd () {
+ touch "$BACKUP"
+ if $base_present
+ then
+ "$merge_tool_path" -X --show-merged-pane \
+ -R 'Accel.SaveAsMerged: "Ctrl-S"' \
+ -R 'Accel.Search: "Ctrl+F"' \
+ -R 'Accel.SearchForward: "Ctrl-G"' \
+ --merged-file "$MERGED" "$LOCAL" "$BASE" "$REMOTE"
+ else
+ "$merge_tool_path" -X $extra \
+ -R 'Accel.SaveAsMerged: "Ctrl-S"' \
+ -R 'Accel.Search: "Ctrl+F"' \
+ -R 'Accel.SearchForward: "Ctrl-G"' \
+ --merged-file "$MERGED" "$LOCAL" "$REMOTE"
+ fi
+ check_unchanged
+}
--
1.7.6.476.g57292
^ permalink raw reply related [flat|nested] 18+ messages in thread* [PATCH da/difftool-mergtool-refactor] Makefile: fix permissions of mergetools/ checked out with permissive umask
2011-08-18 7:23 ` [PATCH 3/4] mergetool--lib: Refactor tools into separate files David Aguilar
@ 2011-10-09 9:17 ` Jonathan Nieder
2011-10-09 11:43 ` Jonathan Nieder
0 siblings, 1 reply; 18+ messages in thread
From: Jonathan Nieder @ 2011-10-09 9:17 UTC (permalink / raw)
To: David Aguilar
Cc: Junio C Hamano, git, Tanguy Ortolo, Charles Bailey,
Sebastian Schuberth
Ever since mergetool--lib was split into multiple files in
v1.7.7-rc0~3^2~1 (2011-08-18), the Makefile takes care to reset umask
and use tar --no-owner when installing merge tool definitions to
$(gitexecdir)/mergetools/. Unfortunately it does not take into
account the possibility that the permission bits of the files being
copied might already be wrong.
Rather than fixing the "tar" incantation and making it even more
complicated, let's just use the "install" utility. This only means
losing the ability to install executables and subdirectories of
mergetools/, which wasn't used.
Noticed by installing from a copy of git checked out with umask 002.
Compare v1.6.0.3~81^2 (Fix permission bits on sources checked out with
an overtight umask, 2008-08-21).
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
David Aguilar wrote:
> +++ b/Makefile
[...]
> @@ -2266,6 +2274,9 @@ install: all
> $(INSTALL) -m 644 $(SCRIPT_LIB) '$(DESTDIR_SQ)$(gitexec_instdir_SQ)'
> $(INSTALL) $(install_bindir_programs) '$(DESTDIR_SQ)$(bindir_SQ)'
> $(MAKE) -C templates DESTDIR='$(DESTDIR_SQ)' install
> + $(INSTALL) -d -m 755 '$(DESTDIR_SQ)$(mergetools_instdir_SQ)'
> + (cd mergetools && $(TAR) cf - .) | \
> + (cd '$(DESTDIR_SQ)$(mergetools_instdir_SQ)' && umask 022 && $(TAR) xof -)
Last month I tried this out and found that, strangely, my files under
/usr/lib/git-core/mergetools/ had the g+w bit set. Leading me to
wonder: does the "umask" here have any effect at all?
Since debian/rules install is run as root, the default is for tar to
act as thought --preserve-permissions were passed, so the umask when
running "tar" is not relevant. Luckily I think "tar" is overkill
here, anyway.
Thoughts? Sorry to have taken so long to send this out.
Makefile | 3 +--
1 files changed, 1 insertions(+), 2 deletions(-)
diff --git a/Makefile b/Makefile
index 1e91b19c..e27755e7 100644
--- a/Makefile
+++ b/Makefile
@@ -2275,8 +2275,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)'
- (cd mergetools && $(TAR) cf - .) | \
- (cd '$(DESTDIR_SQ)$(mergetools_instdir_SQ)' && umask 022 && $(TAR) xof -)
+ $(INSTALL) -m 644 mergetools/* '$(DESTDIR_SQ)$(mergetools_instdir_SQ)'
ifndef NO_PERL
$(MAKE) -C perl prefix='$(prefix_SQ)' DESTDIR='$(DESTDIR_SQ)' install
$(MAKE) -C gitweb install
--
1.7.7.rc1
^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH da/difftool-mergtool-refactor] Makefile: fix permissions of mergetools/ checked out with permissive umask
2011-10-09 9:17 ` [PATCH da/difftool-mergtool-refactor] Makefile: fix permissions of mergetools/ checked out with permissive umask Jonathan Nieder
@ 2011-10-09 11:43 ` Jonathan Nieder
2011-10-09 18:26 ` David Aguilar
0 siblings, 1 reply; 18+ messages in thread
From: Jonathan Nieder @ 2011-10-09 11:43 UTC (permalink / raw)
To: David Aguilar
Cc: Junio C Hamano, git, Tanguy Ortolo, Charles Bailey,
Sebastian Schuberth
Jonathan Nieder wrote:
> ---
[...]
> Since debian/rules install is run as root, the default is for tar to
> act as thought --preserve-permissions were passed
I should have said: "when 'make install' is run as root, ...".
Typically people building git for private use would run "make install"
as root when installing to /usr/local, but as an unprivileged user
when installing to $HOME. The RPM packaging runs "make install"
without special privileges and the Debian packaging runs it as (fake)
root, iirc.
Sorry for the lack of clarity.
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH da/difftool-mergtool-refactor] Makefile: fix permissions of mergetools/ checked out with permissive umask
2011-10-09 11:43 ` Jonathan Nieder
@ 2011-10-09 18:26 ` David Aguilar
2011-10-09 21:47 ` Jonathan Nieder
2011-10-11 0:00 ` Junio C Hamano
0 siblings, 2 replies; 18+ messages in thread
From: David Aguilar @ 2011-10-09 18:26 UTC (permalink / raw)
To: Jonathan Nieder
Cc: Junio C Hamano, git@vger.kernel.org, Tanguy Ortolo,
Charles Bailey, SebastianSchuberth
On Oct 9, 2011, at 4:43 AM, Jonathan Nieder <jrnieder@gmail.com> wrote:
> Jonathan Nieder wrote:
>
>> ---
> [...]
>> Since debian/rules install is run as root, the default is for tar to
>> act as thought --preserve-permissions were passed
>
> I should have said: "when 'make install' is run as root, ...".
>
> Typically people building git for private use would run "make install"
> as root when installing to /usr/local, but as an unprivileged user
> when installing to $HOME. The RPM packaging runs "make install"
> without special privileges and the Debian packaging runs it as (fake)
> root, iirc.
>
> Sorry for the lack of clarity.
thanks. I agree that the tar is overkill. I think I copied that snippet from templates/makefile. does that have the same bug?
--
David (mobile)
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH da/difftool-mergtool-refactor] Makefile: fix permissions of mergetools/ checked out with permissive umask
2011-10-09 18:26 ` David Aguilar
@ 2011-10-09 21:47 ` Jonathan Nieder
2011-10-11 0:00 ` Junio C Hamano
1 sibling, 0 replies; 18+ messages in thread
From: Jonathan Nieder @ 2011-10-09 21:47 UTC (permalink / raw)
To: David Aguilar
Cc: Junio C Hamano, git@vger.kernel.org, Tanguy Ortolo,
Charles Bailey, SebastianSchuberth
David Aguilar wrote:
> thanks. I agree that the tar is overkill. I think I copied that
> snippet from templates/makefile. does that have the same bug?
It did have a similar bug before, but it was fixed (in a different
way) by v1.6.0.3~81^2 (Fix permission bits on sources checked out with
an overtight umask, 2008-08-21). The main difference from the
mergetools/ case is that the blt/ directory is populated at build
time.
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH da/difftool-mergtool-refactor] Makefile: fix permissions of mergetools/ checked out with permissive umask
2011-10-09 18:26 ` David Aguilar
2011-10-09 21:47 ` Jonathan Nieder
@ 2011-10-11 0:00 ` Junio C Hamano
1 sibling, 0 replies; 18+ messages in thread
From: Junio C Hamano @ 2011-10-11 0:00 UTC (permalink / raw)
To: David Aguilar
Cc: Jonathan Nieder, git@vger.kernel.org, Tanguy Ortolo,
Charles Bailey, SebastianSchuberth
David Aguilar <davvid@gmail.com> writes:
> thanks. I agree that the tar is overkill. I think I copied that snippet
> from templates/makefile. does that have the same bug?
Likely, as I recall that I wrote the installation of templates in an
expecially sloppy way, thinking somebody would fix it in short order
anyway. Instead the sloppiness seems to have been copied and pasted to
make things worse...
Thanks for fixing the mess---all's well that ends well.
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH 4/4] mergetools/meld: Use '--output' when available
2011-08-18 7:23 [PATCH 0/4] Refactor git-mergetool--lib.sh David Aguilar
` (2 preceding siblings ...)
2011-08-18 7:23 ` [PATCH 3/4] mergetool--lib: Refactor tools into separate files David Aguilar
@ 2011-08-18 7:23 ` David Aguilar
2011-08-18 8:13 ` Jonathan Nieder
2011-08-19 9:14 ` [PATCH v2 " David Aguilar
2011-08-18 7:57 ` [PATCH 0/4] Refactor git-mergetool--lib.sh Sebastian Schuberth
4 siblings, 2 replies; 18+ messages in thread
From: David Aguilar @ 2011-08-18 7:23 UTC (permalink / raw)
To: Junio C Hamano
Cc: git, Jonathan Nieder, Tanguy Ortolo, Charles Bailey,
Sebastian Schuberth
meld 1.5.0 and newer allow the output file to be specified
when merging multiple files. Check the meld version and use
the '--output' option when available.
Signed-off-by: David Aguilar <davvid@gmail.com>
---
mergetools/meld | 33 ++++++++++++++++++++++++++++++++-
1 files changed, 32 insertions(+), 1 deletions(-)
diff --git a/mergetools/meld b/mergetools/meld
index 73d70ae..1923797 100644
--- a/mergetools/meld
+++ b/mergetools/meld
@@ -4,6 +4,37 @@ diff_cmd () {
merge_cmd () {
touch "$BACKUP"
- "$merge_tool_path" "$LOCAL" "$MERGED" "$REMOTE"
+ if test "$meld_has_output_option" = true
+ then
+ "$merge_tool_path" --output "$MERGED" \
+ "$BASE" "$LOCAL" "$REMOTE"
+ else
+ "$merge_tool_path" "$LOCAL" "$MERGED" "$REMOTE"
+ fi
check_unchanged
}
+
+check_meld_version () {
+ if test -n "$meld_version_checked"
+ then
+ return
+ fi
+ # Whether 'meld --output <file>' is supported
+ meld_has_output_option=false
+
+ # Filter meld --version to contain numbers and dots only
+ meld="$(meld --version 2>/dev/null | sed -e 's/[^0-9.]\+//g')"
+ meld="${meld:-0.0.0}"
+
+ meld_major="$(expr "$meld" : '\([0-9]\{1,\}\)' || echo 0)"
+ meld_minor="$(expr "$meld" : '[0-9]\{1,\}\.\([0-9]\{1,\}\)' || echo 0)"
+
+ # 'meld --output <file>' was introduced in meld 1.5.0
+ if test "$meld_major" -gt 1 ||
+ (test "$meld_major" = 1 && test "$meld_minor" -ge 5)
+ then
+ meld_has_output_option=true
+ fi
+ meld_version_checked=true
+}
+check_meld_version
--
1.7.6.476.g57292
^ permalink raw reply related [flat|nested] 18+ messages in thread* Re: [PATCH 4/4] mergetools/meld: Use '--output' when available
2011-08-18 7:23 ` [PATCH 4/4] mergetools/meld: Use '--output' when available David Aguilar
@ 2011-08-18 8:13 ` Jonathan Nieder
2011-08-18 9:24 ` David Aguilar
2011-08-19 9:14 ` [PATCH v2 " David Aguilar
1 sibling, 1 reply; 18+ messages in thread
From: Jonathan Nieder @ 2011-08-18 8:13 UTC (permalink / raw)
To: David Aguilar
Cc: Junio C Hamano, git, Tanguy Ortolo, Charles Bailey,
Sebastian Schuberth
David Aguilar wrote:
> use the '--output' option when available.
Yay. :)
> --- a/mergetools/meld
> +++ b/mergetools/meld
> @@ -4,6 +4,37 @@ diff_cmd () {
>
> merge_cmd () {
> touch "$BACKUP"
> - "$merge_tool_path" "$LOCAL" "$MERGED" "$REMOTE"
> + if test "$meld_has_output_option" = true
> + then
> + "$merge_tool_path" --output "$MERGED" \
> + "$BASE" "$LOCAL" "$REMOTE"
Shouldn't this be "$LOCAL" "$BASE" "$REMOTE"?
> + else
> + "$merge_tool_path" "$LOCAL" "$MERGED" "$REMOTE"
> + fi
> check_unchanged
I wonder if the version test could be made a little simpler (perhaps
to cope better if future versions use a different numbering system):
if "$merge_tool_path" --output /dev/null --help >/dev/null 2>&1
then
"$merge_tool_path" --output ...
else
"$merge_tool_path" "$LOCAL" ...
fi
Forgive my ignorance: is this function likely to be called in a loop?
If so, it makes sense to precompute or cache the result of detection,
like you already do.
check_meld_for_output_option () {
if ...
then
meld_has_output_option=true
else
meld_has_output_option=false
fi
}
merge_cmd () {
if test -z "${meld_has_output_option:+set}"
then
check_meld_for_output_option
fi
if test "$meld_has_output_option" = true
then
...
[...]
> + # Filter meld --version to contain numbers and dots only
> + meld="$(meld --version 2>/dev/null | sed -e 's/[^0-9.]\+//g')"
\+ is not a BRE. If parsing version numbers seems like the right
thing to do, maybe "tr -cd 0-9."?
> + meld="${meld:-0.0.0}"
> +
> + meld_major="$(expr "$meld" : '\([0-9]\{1,\}\)' || echo 0)"
> + meld_minor="$(expr "$meld" : '[0-9]\{1,\}\.\([0-9]\{1,\}\)' || echo 0)"
I think git avoids \{m,n\} ranges where possible (for portability).
This could be:
meld_major=${meld%%.*}
meld_nonmajor=${meld#${meld_major}.}
meld_minor=${meld_nonmajor%%.*}
or:
case $meld in
[2-9].* | [1-9][0-9]* | 1.[5-9]* | 1.[1-9][0-9]*) # >= 1.5.0
meld_has_output_option=true ;;
*)
meld_has_output_option=false ;;
esac
It's nice how self-contained this can be now that it's in its own
file. Thanks.
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH 4/4] mergetools/meld: Use '--output' when available
2011-08-18 8:13 ` Jonathan Nieder
@ 2011-08-18 9:24 ` David Aguilar
0 siblings, 0 replies; 18+ messages in thread
From: David Aguilar @ 2011-08-18 9:24 UTC (permalink / raw)
To: Jonathan Nieder
Cc: Junio C Hamano, git, Tanguy Ortolo, Charles Bailey,
Sebastian Schuberth
On Thu, Aug 18, 2011 at 03:13:10AM -0500, Jonathan Nieder wrote:
> David Aguilar wrote:
>
> > use the '--output' option when available.
>
> Yay. :)
>
> > --- a/mergetools/meld
> > +++ b/mergetools/meld
> > @@ -4,6 +4,37 @@ diff_cmd () {
> >
> > merge_cmd () {
> > touch "$BACKUP"
> > - "$merge_tool_path" "$LOCAL" "$MERGED" "$REMOTE"
> > + if test "$meld_has_output_option" = true
> > + then
> > + "$merge_tool_path" --output "$MERGED" \
> > + "$BASE" "$LOCAL" "$REMOTE"
>
> Shouldn't this be "$LOCAL" "$BASE" "$REMOTE"?
Yup, thanks.
>
> > + else
> > + "$merge_tool_path" "$LOCAL" "$MERGED" "$REMOTE"
> > + fi
> > check_unchanged
>
> I wonder if the version test could be made a little simpler (perhaps
> to cope better if future versions use a different numbering system):
>
> if "$merge_tool_path" --output /dev/null --help >/dev/null 2>&1
> then
> "$merge_tool_path" --output ...
> else
> "$merge_tool_path" "$LOCAL" ...
> fi
>
> Forgive my ignorance: is this function likely to be called in a loop?
> If so, it makes sense to precompute or cache the result of detection,
> like you already do.
Yes, it is called in a loop...
> check_meld_for_output_option () {
> if ...
> then
> meld_has_output_option=true
> else
> meld_has_output_option=false
> fi
> }
>
> merge_cmd () {
> if test -z "${meld_has_output_option:+set}"
> then
> check_meld_for_output_option
> fi
>
> if test "$meld_has_output_option" = true
> then
> ...
>
>
> [...]
> > + # Filter meld --version to contain numbers and dots only
> > + meld="$(meld --version 2>/dev/null | sed -e 's/[^0-9.]\+//g')"
>
> \+ is not a BRE. If parsing version numbers seems like the right
> thing to do, maybe "tr -cd 0-9."?
>
> > + meld="${meld:-0.0.0}"
> > +
> > + meld_major="$(expr "$meld" : '\([0-9]\{1,\}\)' || echo 0)"
> > + meld_minor="$(expr "$meld" : '[0-9]\{1,\}\.\([0-9]\{1,\}\)' || echo 0)"
>
> I think git avoids \{m,n\} ranges where possible (for portability).
> This could be:
>
> meld_major=${meld%%.*}
> meld_nonmajor=${meld#${meld_major}.}
> meld_minor=${meld_nonmajor%%.*}
>
> or:
>
> case $meld in
> [2-9].* | [1-9][0-9]* | 1.[5-9]* | 1.[1-9][0-9]*) # >= 1.5.0
> meld_has_output_option=true ;;
> *)
> meld_has_output_option=false ;;
> esac
>
> It's nice how self-contained this can be now that it's in its own
> file. Thanks.
Right, I was using \{1,\} since that's what CodingStyle said to
use instead of \+ (which I forgot to fixup above as you saw).
The case statement is nice and simple enough to understand.
By doing meld --output /dev/null --help we're relying on
older versions blowing up with --output and --help returning
exit status 0. That seems pretty reasonable.
The case statement does seem sufficient but just trying
--output and seeing what happens is even simpler.
Your example also uses $merge_tool_path, which I forgot to do,
so I'll be sure to include that too.
I'll wait until tomorrow to see if there are any more comments
and reroll.
Thanks,
--
David
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH v2 4/4] mergetools/meld: Use '--output' when available
2011-08-18 7:23 ` [PATCH 4/4] mergetools/meld: Use '--output' when available David Aguilar
2011-08-18 8:13 ` Jonathan Nieder
@ 2011-08-19 9:14 ` David Aguilar
2011-08-19 22:58 ` Junio C Hamano
1 sibling, 1 reply; 18+ messages in thread
From: David Aguilar @ 2011-08-19 9:14 UTC (permalink / raw)
To: Junio C Hamano
Cc: git, Jonathan Nieder, Tanguy Ortolo, Charles Bailey,
Sebastian Schuberth
meld 1.5.0 and newer allow the output file to be specified
when merging multiple files. Check whether the meld command
supports '--output' and use it when available.
Signed-off-by: David Aguilar <davvid@gmail.com>
---
mergetools/meld | 25 ++++++++++++++++++++++++-
1 files changed, 24 insertions(+), 1 deletions(-)
Changes since v1 with help from Jonathan Nieder:
Use $LOCAL $BASE REMOTE when calling meld.
Avoid shell portability issues by using the return status.
of --output instead of parsing --version.
diff --git a/mergetools/meld b/mergetools/meld
index 73d70ae..eaa115c 100644
--- a/mergetools/meld
+++ b/mergetools/meld
@@ -3,7 +3,30 @@ diff_cmd () {
}
merge_cmd () {
+ if test -z "${meld_has_output_option:+set}"
+ then
+ check_meld_for_output_version
+ fi
touch "$BACKUP"
- "$merge_tool_path" "$LOCAL" "$MERGED" "$REMOTE"
+ if test "$meld_has_output_option" = true
+ then
+ "$merge_tool_path" --output "$MERGED" \
+ "$LOCAL" "$BASE" "$REMOTE"
+ else
+ "$merge_tool_path" "$LOCAL" "$MERGED" "$REMOTE"
+ fi
check_unchanged
}
+
+# Check whether 'meld --output <file>' is supported
+check_meld_for_output_version () {
+ meld_path="$(git config mergetool.meld.path)"
+ meld_path="${meld_path:-meld}"
+
+ if "$meld_path" --output /dev/null --help >/dev/null 2>&1
+ then
+ meld_has_output_option=true
+ else
+ meld_has_output_option=false
+ fi
+}
--
1.7.6.553.g1899
^ permalink raw reply related [flat|nested] 18+ messages in thread* Re: [PATCH v2 4/4] mergetools/meld: Use '--output' when available
2011-08-19 9:14 ` [PATCH v2 " David Aguilar
@ 2011-08-19 22:58 ` Junio C Hamano
0 siblings, 0 replies; 18+ messages in thread
From: Junio C Hamano @ 2011-08-19 22:58 UTC (permalink / raw)
To: David Aguilar
Cc: git, Jonathan Nieder, Tanguy Ortolo, Charles Bailey,
Sebastian Schuberth
David Aguilar <davvid@gmail.com> writes:
> meld 1.5.0 and newer allow the output file to be specified
> when merging multiple files. Check whether the meld command
> supports '--output' and use it when available.
>
> Signed-off-by: David Aguilar <davvid@gmail.com>
> ---
> mergetools/meld | 25 ++++++++++++++++++++++++-
> 1 files changed, 24 insertions(+), 1 deletions(-)
>
> Changes since v1 with help from Jonathan Nieder:
Thanks, both of you. Queued all patches in the series.
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 0/4] Refactor git-mergetool--lib.sh
2011-08-18 7:23 [PATCH 0/4] Refactor git-mergetool--lib.sh David Aguilar
` (3 preceding siblings ...)
2011-08-18 7:23 ` [PATCH 4/4] mergetools/meld: Use '--output' when available David Aguilar
@ 2011-08-18 7:57 ` Sebastian Schuberth
4 siblings, 0 replies; 18+ messages in thread
From: Sebastian Schuberth @ 2011-08-18 7:57 UTC (permalink / raw)
To: David Aguilar
Cc: Junio C Hamano, git, Jonathan Nieder, Tanguy Ortolo,
Charles Bailey
On Thu, Aug 18, 2011 at 09:23, David Aguilar <davvid@gmail.com> wrote:
> This series splits the git-mergetool--lib.sh file into separate
> tool scriptlets that define the commands for diff and merge.
Very nice as it simplifies adding new merge tools to adding a single
file with little code inside. Thanks!
--
Sebastian Schuberth
^ permalink raw reply [flat|nested] 18+ messages in thread