* (unknown),
@ 2009-03-30 5:03 David Aguilar
2009-03-30 5:03 ` [PATCH 1/8] mergetool: use tabs consistently David Aguilar
2009-03-30 7:02 ` Markus Heidelberg
0 siblings, 2 replies; 19+ messages in thread
From: David Aguilar @ 2009-03-30 5:03 UTC (permalink / raw)
To: gitster; +Cc: git
As promised, here is the patch series that removes the duplicate
code between git-difftool and git-mergetool.
This is based on top of Junio's "pu" branch and is a
continuation of the recent difftool series.
I created a new git-sh-tools shell lib for holding the
common functions. If anyone thinks I should have placed the
functions in git-sh-setup instead then just let me know.
Here's a total diffstat. If it wasn't for the documentation
and replacing the mixed spaces/tabs with all-tabs in
git-mergetool then we would have seen a lot more
happy removals.
.gitignore | 1 +
Documentation/git-sh-tools.txt | 52 ++++
Makefile | 1 +
command-list.txt | 1 +
git-difftool-helper.sh | 112 +-------
git-mergetool.sh | 639 ++++++++++++++++------------------------
git-sh-tools.sh | 181 ++++++++++++
7 files changed, 506 insertions(+), 481 deletions(-)
GIT:
From: David Aguilar <davvid@gmail.com>
Subject: Refactor git-{diff,merge}tool to remove duplicate code
In-Reply-To:
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH 1/8] mergetool: use tabs consistently
2009-03-30 5:03 (unknown), David Aguilar
@ 2009-03-30 5:03 ` David Aguilar
2009-03-30 5:03 ` [PATCH 2/8] mergetool: use $( ... ) instead of `backticks` David Aguilar
2009-03-30 8:44 ` [PATCH 1/8] mergetool: use tabs consistently Junio C Hamano
2009-03-30 7:02 ` Markus Heidelberg
1 sibling, 2 replies; 19+ messages in thread
From: David Aguilar @ 2009-03-30 5:03 UTC (permalink / raw)
To: gitster; +Cc: git, David Aguilar
This makes mergetool use hard tabs throughout.
Signed-off-by: David Aguilar <davvid@gmail.com>
---
git-mergetool.sh | 694 +++++++++++++++++++++++++++--------------------------
1 files changed, 354 insertions(+), 340 deletions(-)
diff --git a/git-mergetool.sh b/git-mergetool.sh
index 87fa88a..cfee28e 100755
--- a/git-mergetool.sh
+++ b/git-mergetool.sh
@@ -16,336 +16,351 @@ require_work_tree
# Returns true if the mode reflects a symlink
is_symlink () {
- test "$1" = 120000
+ test "$1" = 120000
}
local_present () {
- test -n "$local_mode"
+ test -n "$local_mode"
}
remote_present () {
- test -n "$remote_mode"
+ test -n "$remote_mode"
}
base_present () {
- test -n "$base_mode"
+ test -n "$base_mode"
}
cleanup_temp_files () {
- if test "$1" = --save-backup ; then
- mv -- "$BACKUP" "$MERGED.orig"
- rm -f -- "$LOCAL" "$REMOTE" "$BASE"
- else
- rm -f -- "$LOCAL" "$REMOTE" "$BASE" "$BACKUP"
- fi
+ if test "$1" = --save-backup ; then
+ mv -- "$BACKUP" "$MERGED.orig"
+ rm -f -- "$LOCAL" "$REMOTE" "$BASE"
+ else
+ rm -f -- "$LOCAL" "$REMOTE" "$BASE" "$BACKUP"
+ fi
}
describe_file () {
- mode="$1"
- branch="$2"
- file="$3"
-
- printf " {%s}: " "$branch"
- if test -z "$mode"; then
- echo "deleted"
- elif is_symlink "$mode" ; then
- echo "a symbolic link -> '$(cat "$file")'"
- else
- if base_present; then
- echo "modified"
+ mode="$1"
+ branch="$2"
+ file="$3"
+
+ printf " {%s}: " "$branch"
+ if test -z "$mode"; then
+ echo "deleted"
+ elif is_symlink "$mode" ; then
+ echo "a symbolic link -> '$(cat "$file")'"
else
- echo "created"
+ if base_present; then
+ echo "modified"
+ else
+ echo "created"
+ fi
fi
- fi
}
resolve_symlink_merge () {
- while true; do
- printf "Use (l)ocal or (r)emote, or (a)bort? "
- read ans
- case "$ans" in
- [lL]*)
- git checkout-index -f --stage=2 -- "$MERGED"
- git add -- "$MERGED"
- cleanup_temp_files --save-backup
- return 0
- ;;
- [rR]*)
- git checkout-index -f --stage=3 -- "$MERGED"
- git add -- "$MERGED"
- cleanup_temp_files --save-backup
- return 0
- ;;
- [aA]*)
- return 1
- ;;
- esac
+ while true; do
+ printf "Use (l)ocal or (r)emote, or (a)bort? "
+ read ans
+ case "$ans" in
+ [lL]*)
+ git checkout-index -f --stage=2 -- "$MERGED"
+ git add -- "$MERGED"
+ cleanup_temp_files --save-backup
+ return 0
+ ;;
+ [rR]*)
+ git checkout-index -f --stage=3 -- "$MERGED"
+ git add -- "$MERGED"
+ cleanup_temp_files --save-backup
+ return 0
+ ;;
+ [aA]*)
+ return 1
+ ;;
+ esac
done
}
resolve_deleted_merge () {
- while true; do
- if base_present; then
- printf "Use (m)odified or (d)eleted file, or (a)bort? "
- else
- printf "Use (c)reated or (d)eleted file, or (a)bort? "
- fi
- read ans
- case "$ans" in
- [mMcC]*)
- git add -- "$MERGED"
- cleanup_temp_files --save-backup
- return 0
- ;;
- [dD]*)
- git rm -- "$MERGED" > /dev/null
- cleanup_temp_files
- return 0
- ;;
- [aA]*)
- return 1
- ;;
- esac
+ while true; do
+ if base_present; then
+ printf "Use (m)odified or (d)eleted file, or (a)bort? "
+ else
+ printf "Use (c)reated or (d)eleted file, or (a)bort? "
+ fi
+ read ans
+ case "$ans" in
+ [mMcC]*)
+ git add -- "$MERGED"
+ cleanup_temp_files --save-backup
+ return 0
+ ;;
+ [dD]*)
+ git rm -- "$MERGED" > /dev/null
+ cleanup_temp_files
+ return 0
+ ;;
+ [aA]*)
+ return 1
+ ;;
+ esac
done
}
check_unchanged () {
- if test "$MERGED" -nt "$BACKUP" ; then
- status=0;
- else
- while true; do
- echo "$MERGED seems unchanged."
- printf "Was the merge successful? [y/n] "
- read answer < /dev/tty
- case "$answer" in
- y*|Y*) status=0; break ;;
- n*|N*) status=1; break ;;
- esac
- done
- fi
+ if test "$MERGED" -nt "$BACKUP" ; then
+ status=0;
+ else
+ while true; do
+ echo "$MERGED seems unchanged."
+ printf "Was the merge successful? [y/n] "
+ read answer < /dev/tty
+ case "$answer" in
+ y*|Y*) status=0; break ;;
+ n*|N*) status=1; break ;;
+ esac
+ done
+ fi
}
checkout_staged_file () {
- tmpfile=$(expr "$(git checkout-index --temp --stage="$1" "$2")" : '\([^ ]*\) ')
+ checkouttmp=$(git checkout-index --temp --stage="$1" "$2")
+ tmpfile=$(expr "$checkouttmp" : '\([^ ]*\) ')
- if test $? -eq 0 -a -n "$tmpfile" ; then
- mv -- "$(git rev-parse --show-cdup)$tmpfile" "$3"
- fi
+ if test $? -eq 0 -a -n "$tmpfile" ; then
+ mv -- "$(git rev-parse --show-cdup)$tmpfile" "$3"
+ fi
}
merge_file () {
- MERGED="$1"
+ MERGED="$1"
- f=`git ls-files -u -- "$MERGED"`
- if test -z "$f" ; then
- if test ! -f "$MERGED" ; then
- echo "$MERGED: file not found"
- else
- echo "$MERGED: file does not need merging"
+ f=`git ls-files -u -- "$MERGED"`
+ if test -z "$f" ; then
+ if test ! -f "$MERGED" ; then
+ echo "$MERGED: file not found"
+ else
+ echo "$MERGED: file does not need merging"
+ fi
+ return 1
fi
- return 1
- fi
-
- ext="$$$(expr "$MERGED" : '.*\(\.[^/]*\)$')"
- BACKUP="./$MERGED.BACKUP.$ext"
- LOCAL="./$MERGED.LOCAL.$ext"
- REMOTE="./$MERGED.REMOTE.$ext"
- BASE="./$MERGED.BASE.$ext"
- mv -- "$MERGED" "$BACKUP"
- cp -- "$BACKUP" "$MERGED"
-
- base_mode=`git ls-files -u -- "$MERGED" | awk '{if ($3==1) print $1;}'`
- local_mode=`git ls-files -u -- "$MERGED" | awk '{if ($3==2) print $1;}'`
- remote_mode=`git ls-files -u -- "$MERGED" | awk '{if ($3==3) print $1;}'`
+ ext="$$$(expr "$MERGED" : '.*\(\.[^/]*\)$')"
+ BACKUP="./$MERGED.BACKUP.$ext"
+ LOCAL="./$MERGED.LOCAL.$ext"
+ REMOTE="./$MERGED.REMOTE.$ext"
+ BASE="./$MERGED.BASE.$ext"
+
+ mv -- "$MERGED" "$BACKUP"
+ cp -- "$BACKUP" "$MERGED"
+
+ base_mode=`git ls-files -u -- "$MERGED" | awk '{if ($3==1) print $1;}'`
+ local_mode=`git ls-files -u -- "$MERGED" | awk '{if ($3==2) print $1;}'`
+ remote_mode=`git ls-files -u -- "$MERGED" | awk '{if ($3==3) print $1;}'`
+
+ base_present && checkout_staged_file 1 "$MERGED" "$BASE"
+ local_present && checkout_staged_file 2 "$MERGED" "$LOCAL"
+ remote_present && checkout_staged_file 3 "$MERGED" "$REMOTE"
+
+ if test -z "$local_mode" -o -z "$remote_mode"; then
+ echo "Deleted merge conflict for '$MERGED':"
+ describe_file "$local_mode" "local" "$LOCAL"
+ describe_file "$remote_mode" "remote" "$REMOTE"
+ resolve_deleted_merge
+ return
+ fi
- base_present && checkout_staged_file 1 "$MERGED" "$BASE"
- local_present && checkout_staged_file 2 "$MERGED" "$LOCAL"
- remote_present && checkout_staged_file 3 "$MERGED" "$REMOTE"
+ if is_symlink "$local_mode" || is_symlink "$remote_mode"; then
+ echo "Symbolic link merge conflict for '$MERGED':"
+ describe_file "$local_mode" "local" "$LOCAL"
+ describe_file "$remote_mode" "remote" "$REMOTE"
+ resolve_symlink_merge
+ return
+ fi
- if test -z "$local_mode" -o -z "$remote_mode"; then
- echo "Deleted merge conflict for '$MERGED':"
+ echo "Normal merge conflict for '$MERGED':"
describe_file "$local_mode" "local" "$LOCAL"
describe_file "$remote_mode" "remote" "$REMOTE"
- resolve_deleted_merge
- return
- fi
+ if "$prompt" = true; then
+ printf "Hit return to start merge resolution tool (%s): " "$merge_tool"
+ read ans
+ fi
- if is_symlink "$local_mode" || is_symlink "$remote_mode"; then
- echo "Symbolic link merge conflict for '$MERGED':"
- describe_file "$local_mode" "local" "$LOCAL"
- describe_file "$remote_mode" "remote" "$REMOTE"
- resolve_symlink_merge
- return
- fi
-
- echo "Normal merge conflict for '$MERGED':"
- describe_file "$local_mode" "local" "$LOCAL"
- describe_file "$remote_mode" "remote" "$REMOTE"
- if "$prompt" = true; then
- printf "Hit return to start merge resolution tool (%s): " "$merge_tool"
- read ans
- fi
-
- case "$merge_tool" in
+ case "$merge_tool" in
kdiff3)
- 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=$?
- ;;
+ 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=$?
+ ;;
tkdiff)
- if base_present ; then
- "$merge_tool_path" -a "$BASE" -o "$MERGED" "$LOCAL" "$REMOTE"
- else
- "$merge_tool_path" -o "$MERGED" "$LOCAL" "$REMOTE"
- fi
- status=$?
- ;;
+ if base_present ; then
+ "$merge_tool_path" -a "$BASE" -o "$MERGED" "$LOCAL" "$REMOTE"
+ else
+ "$merge_tool_path" -o "$MERGED" "$LOCAL" "$REMOTE"
+ fi
+ status=$?
+ ;;
meld)
- touch "$BACKUP"
- "$merge_tool_path" "$LOCAL" "$MERGED" "$REMOTE"
- check_unchanged
- ;;
+ touch "$BACKUP"
+ "$merge_tool_path" "$LOCAL" "$MERGED" "$REMOTE"
+ check_unchanged
+ ;;
vimdiff)
- touch "$BACKUP"
- "$merge_tool_path" -c "wincmd l" "$LOCAL" "$MERGED" "$REMOTE"
- check_unchanged
- ;;
+ touch "$BACKUP"
+ "$merge_tool_path" -c "wincmd l" "$LOCAL" "$MERGED" "$REMOTE"
+ check_unchanged
+ ;;
gvimdiff)
- touch "$BACKUP"
- "$merge_tool_path" -c "wincmd l" -f "$LOCAL" "$MERGED" "$REMOTE"
- check_unchanged
- ;;
+ touch "$BACKUP"
+ "$merge_tool_path" -c "wincmd l" -f "$LOCAL" "$MERGED" "$REMOTE"
+ check_unchanged
+ ;;
xxdiff)
- 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 --show-merged-pane \
- -R 'Accel.SaveAsMerged: "Ctrl-S"' \
- -R 'Accel.Search: "Ctrl+F"' \
- -R 'Accel.SearchForward: "Ctrl-G"' \
- --merged-file "$MERGED" "$LOCAL" "$REMOTE"
- fi
- check_unchanged
- ;;
+ 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 --show-merged-pane \
+ -R 'Accel.SaveAsMerged: "Ctrl-S"' \
+ -R 'Accel.Search: "Ctrl+F"' \
+ -R 'Accel.SearchForward: "Ctrl-G"' \
+ --merged-file "$MERGED" "$LOCAL" "$REMOTE"
+ fi
+ check_unchanged
+ ;;
opendiff)
- 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
- ;;
+ 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
+ ;;
ecmerge)
- 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
- ;;
+ 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
+ ;;
emerge)
- 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=$?
- ;;
- *)
- if test -n "$merge_tool_cmd"; then
- if test "$merge_tool_trust_exit_code" = "false"; then
- touch "$BACKUP"
- ( eval $merge_tool_cmd )
- check_unchanged
+ if base_present ; then
+ "$merge_tool_path" -f emerge-files-with-ancestor-command \
+ "$LOCAL" "$REMOTE" "$BASE" "$(basename "$MERGED")"
else
- ( eval $merge_tool_cmd )
- status=$?
+ "$merge_tool_path" -f emerge-files-command \
+ "$LOCAL" "$REMOTE" "$(basename "$MERGED")"
fi
- fi
- ;;
- esac
- if test "$status" -ne 0; then
- echo "merge of $MERGED failed" 1>&2
- mv -- "$BACKUP" "$MERGED"
-
- if test "$merge_keep_temporaries" = "false"; then
- cleanup_temp_files
- fi
+ status=$?
+ ;;
+ *)
+ if test -n "$merge_tool_cmd"; then
+ if test "$merge_tool_trust_exit_code" = "false"; then
+ touch "$BACKUP"
+ ( eval $merge_tool_cmd )
+ check_unchanged
+ else
+ ( eval $merge_tool_cmd )
+ status=$?
+ fi
+ fi
+ ;;
+ esac
+ if test "$status" -ne 0; then
+ echo "merge of $MERGED failed" 1>&2
+ mv -- "$BACKUP" "$MERGED"
- return 1
- fi
+ if test "$merge_keep_temporaries" = "false"; then
+ cleanup_temp_files
+ fi
- if test "$merge_keep_backup" = "true"; then
- mv -- "$BACKUP" "$MERGED.orig"
- else
- rm -- "$BACKUP"
- fi
+ return 1
+ fi
+
+ if test "$merge_keep_backup" = "true"; then
+ mv -- "$BACKUP" "$MERGED.orig"
+ else
+ rm -- "$BACKUP"
+ fi
- git add -- "$MERGED"
- cleanup_temp_files
- return 0
+ git add -- "$MERGED"
+ cleanup_temp_files
+ return 0
}
prompt=$(git config --bool mergetool.prompt || echo true)
while test $# != 0
do
- case "$1" in
+ case "$1" in
-t|--tool*)
- case "$#,$1" in
+ case "$#,$1" in
*,*=*)
- merge_tool=`expr "z$1" : 'z-[^=]*=\(.*\)'`
- ;;
+ merge_tool=`expr "z$1" : 'z-[^=]*=\(.*\)'`
+ ;;
1,*)
- usage ;;
+ usage ;;
*)
- merge_tool="$2"
- shift ;;
- esac
- ;;
+ merge_tool="$2"
+ shift ;;
+ esac
+ ;;
-y|--no-prompt)
- prompt=false
- ;;
+ prompt=false
+ ;;
--prompt)
- prompt=true
- ;;
+ prompt=true
+ ;;
--)
- shift
- break
- ;;
+ shift
+ break
+ ;;
-*)
- usage
- ;;
+ usage
+ ;;
*)
- break
- ;;
- esac
- shift
+ break
+ ;;
+ esac
+ shift
done
valid_custom_tool()
{
- merge_tool_cmd="$(git config mergetool.$1.cmd)"
- test -n "$merge_tool_cmd"
+ merge_tool_cmd="$(git config mergetool.$1.cmd)"
+ test -n "$merge_tool_cmd"
}
valid_tool() {
case "$1" in
- kdiff3 | tkdiff | xxdiff | meld | opendiff | emerge | vimdiff | gvimdiff | ecmerge)
+ kdiff3 | tkdiff | xxdiff | meld | opendiff | \
+ emerge | vimdiff | gvimdiff | ecmerge)
;; # happy
*)
if ! valid_custom_tool "$1"; then
@@ -370,117 +385,116 @@ init_merge_tool_path() {
}
prompt_after_failed_merge() {
- while true; do
- printf "Continue merging other unresolved paths (y/n) ? "
- read ans
- case "$ans" in
-
- [yY]*)
- return 0
- ;;
+ while true; do
+ printf "Continue merging other unresolved paths (y/n) ? "
+ read ans
+ case "$ans" in
+ [yY]*)
+ return 0
+ ;;
- [nN]*)
- return 1
- ;;
- esac
- done
+ [nN]*)
+ return 1
+ ;;
+ esac
+ done
}
if test -z "$merge_tool"; then
- merge_tool=`git config merge.tool`
- if test -n "$merge_tool" && ! valid_tool "$merge_tool"; then
- echo >&2 "git config option merge.tool set to unknown tool: $merge_tool"
- echo >&2 "Resetting to default..."
- unset merge_tool
- fi
+ merge_tool=`git config merge.tool`
+ if test -n "$merge_tool" && ! valid_tool "$merge_tool"; then
+ echo >&2 "git config option merge.tool set to unknown tool:" "$merge_tool"
+ echo >&2 "Resetting to default..."
+ unset merge_tool
+ fi
fi
if test -z "$merge_tool" ; then
- if test -n "$DISPLAY"; then
- if test -n "$GNOME_DESKTOP_SESSION_ID" ; then
- merge_tool_candidates="meld kdiff3 tkdiff xxdiff gvimdiff"
- else
- merge_tool_candidates="kdiff3 tkdiff xxdiff meld gvimdiff"
- fi
- fi
- if echo "${VISUAL:-$EDITOR}" | grep 'emacs' > /dev/null 2>&1; then
- merge_tool_candidates="$merge_tool_candidates emerge opendiff vimdiff"
- elif echo "${VISUAL:-$EDITOR}" | grep 'vim' > /dev/null 2>&1; then
- merge_tool_candidates="$merge_tool_candidates vimdiff opendiff emerge"
- else
- merge_tool_candidates="$merge_tool_candidates opendiff emerge vimdiff"
- fi
- echo "merge tool candidates: $merge_tool_candidates"
- for i in $merge_tool_candidates; do
- init_merge_tool_path $i
- if type "$merge_tool_path" > /dev/null 2>&1; then
- merge_tool=$i
- break
- fi
- done
- if test -z "$merge_tool" ; then
- echo "No known merge resolution program available."
- exit 1
- fi
+ if test -n "$DISPLAY"; then
+ if test -n "$GNOME_DESKTOP_SESSION_ID" ; then
+ merge_tool_candidates="meld kdiff3 tkdiff xxdiff gvimdiff"
+ else
+ merge_tool_candidates="kdiff3 tkdiff xxdiff meld gvimdiff"
+ fi
+ fi
+ if echo "${VISUAL:-$EDITOR}" | grep 'emacs' > /dev/null 2>&1; then
+ merge_tool_candidates="$merge_tool_candidates emerge opendiff vimdiff"
+ elif echo "${VISUAL:-$EDITOR}" | grep 'vim' > /dev/null 2>&1; then
+ merge_tool_candidates="$merge_tool_candidates vimdiff opendiff emerge"
+ else
+ merge_tool_candidates="$merge_tool_candidates opendiff emerge vimdiff"
+ fi
+ echo "merge tool candidates: $merge_tool_candidates"
+ for i in $merge_tool_candidates; do
+ init_merge_tool_path $i
+ if type "$merge_tool_path" > /dev/null 2>&1; then
+ merge_tool=$i
+ break
+ fi
+ done
+ if test -z "$merge_tool" ; then
+ echo "No known merge resolution program available."
+ exit 1
+ fi
else
- if ! valid_tool "$merge_tool"; then
- echo >&2 "Unknown merge_tool $merge_tool"
- exit 1
- fi
+ if ! valid_tool "$merge_tool"; then
+ echo >&2 "Unknown merge_tool $merge_tool"
+ exit 1
+ fi
- init_merge_tool_path "$merge_tool"
+ init_merge_tool_path "$merge_tool"
- merge_keep_backup="$(git config --bool merge.keepBackup || echo true)"
- merge_keep_temporaries="$(git config --bool mergetool.keepTemporaries || echo false)"
+ merge_keep_backup="$(git config --bool merge.keepBackup || echo true)"
+ merge_keep_temporaries="$(git config --bool mergetool.keepTemporaries || echo false)"
- if test -z "$merge_tool_cmd" && ! type "$merge_tool_path" > /dev/null 2>&1; then
- echo "The merge tool $merge_tool is not available as '$merge_tool_path'"
- exit 1
- fi
+ if test -z "$merge_tool_cmd" && ! type "$merge_tool_path" > /dev/null 2>&1; then
+ echo "The merge tool $merge_tool is not available as '$merge_tool_path'"
+ exit 1
+ fi
- if ! test -z "$merge_tool_cmd"; then
- merge_tool_trust_exit_code="$(git config --bool mergetool.$merge_tool.trustExitCode || echo false)"
- fi
+ if ! test -z "$merge_tool_cmd"; then
+ merge_tool_trust_exit_code="$(git config --bool mergetool.$merge_tool.trustExitCode || echo false)"
+ fi
fi
last_status=0
rollup_status=0
if test $# -eq 0 ; then
- files=`git ls-files -u | sed -e 's/^[^ ]* //' | sort -u`
- if test -z "$files" ; then
- echo "No files need merging"
- exit 0
- fi
- echo Merging the files: "$files"
- git ls-files -u |
- sed -e 's/^[^ ]* //' |
- sort -u |
- while IFS= read i
- do
- if test $last_status -ne 0; then
- prompt_after_failed_merge < /dev/tty || exit 1
+ files=`git ls-files -u | sed -e 's/^[^ ]* //' | sort -u`
+ if test -z "$files" ; then
+ echo "No files need merging"
+ exit 0
fi
- printf "\n"
- merge_file "$i" < /dev/tty > /dev/tty
- last_status=$?
- if test $last_status -ne 0; then
- rollup_status=1
- fi
- done
+ echo Merging the files: "$files"
+ git ls-files -u |
+ sed -e 's/^[^ ]* //' |
+ sort -u |
+ while IFS= read i
+ do
+ if test $last_status -ne 0; then
+ prompt_after_failed_merge < /dev/tty || exit 1
+ fi
+ printf "\n"
+ merge_file "$i" < /dev/tty > /dev/tty
+ last_status=$?
+ if test $last_status -ne 0; then
+ rollup_status=1
+ fi
+ done
else
- while test $# -gt 0; do
- if test $last_status -ne 0; then
- prompt_after_failed_merge || exit 1
- fi
- printf "\n"
- merge_file "$1"
- last_status=$?
- if test $last_status -ne 0; then
- rollup_status=1
- fi
- shift
- done
+ while test $# -gt 0; do
+ if test $last_status -ne 0; then
+ prompt_after_failed_merge || exit 1
+ fi
+ printf "\n"
+ merge_file "$1"
+ last_status=$?
+ if test $last_status -ne 0; then
+ rollup_status=1
+ fi
+ shift
+ done
fi
exit $rollup_status
--
1.6.1.3
^ permalink raw reply related [flat|nested] 19+ messages in thread
* [PATCH 2/8] mergetool: use $( ... ) instead of `backticks`
2009-03-30 5:03 ` [PATCH 1/8] mergetool: use tabs consistently David Aguilar
@ 2009-03-30 5:03 ` David Aguilar
2009-03-30 5:03 ` [PATCH 3/8] sh-tools: add a git-sh-tools shell helper script David Aguilar
2009-03-30 8:44 ` [PATCH 1/8] mergetool: use tabs consistently Junio C Hamano
1 sibling, 1 reply; 19+ messages in thread
From: David Aguilar @ 2009-03-30 5:03 UTC (permalink / raw)
To: gitster; +Cc: git, David Aguilar
This makes mergetool consistent with Documentation/CodingGuidelines.
Signed-off-by: David Aguilar <davvid@gmail.com>
---
git-mergetool.sh | 16 ++++++++--------
1 files changed, 8 insertions(+), 8 deletions(-)
diff --git a/git-mergetool.sh b/git-mergetool.sh
index cfee28e..5ea126c 100755
--- a/git-mergetool.sh
+++ b/git-mergetool.sh
@@ -138,7 +138,7 @@ checkout_staged_file () {
merge_file () {
MERGED="$1"
- f=`git ls-files -u -- "$MERGED"`
+ f=$(git ls-files -u -- "$MERGED")
if test -z "$f" ; then
if test ! -f "$MERGED" ; then
echo "$MERGED: file not found"
@@ -157,9 +157,9 @@ merge_file () {
mv -- "$MERGED" "$BACKUP"
cp -- "$BACKUP" "$MERGED"
- base_mode=`git ls-files -u -- "$MERGED" | awk '{if ($3==1) print $1;}'`
- local_mode=`git ls-files -u -- "$MERGED" | awk '{if ($3==2) print $1;}'`
- remote_mode=`git ls-files -u -- "$MERGED" | awk '{if ($3==3) print $1;}'`
+ base_mode=$(git ls-files -u -- "$MERGED" | awk '{if ($3==1) print $1;}')
+ local_mode=$(git ls-files -u -- "$MERGED" | awk '{if ($3==2) print $1;}')
+ remote_mode=$(git ls-files -u -- "$MERGED" | awk '{if ($3==3) print $1;}')
base_present && checkout_staged_file 1 "$MERGED" "$BASE"
local_present && checkout_staged_file 2 "$MERGED" "$LOCAL"
@@ -322,7 +322,7 @@ do
-t|--tool*)
case "$#,$1" in
*,*=*)
- merge_tool=`expr "z$1" : 'z-[^=]*=\(.*\)'`
+ merge_tool=$(expr "z$1" : 'z-[^=]*=\(.*\)')
;;
1,*)
usage ;;
@@ -371,7 +371,7 @@ valid_tool() {
}
init_merge_tool_path() {
- merge_tool_path=`git config mergetool.$1.path`
+ merge_tool_path=$(git config mergetool.$1.path)
if test -z "$merge_tool_path" ; then
case "$1" in
emerge)
@@ -401,7 +401,7 @@ prompt_after_failed_merge() {
}
if test -z "$merge_tool"; then
- merge_tool=`git config merge.tool`
+ merge_tool=$(git config merge.tool)
if test -n "$merge_tool" && ! valid_tool "$merge_tool"; then
echo >&2 "git config option merge.tool set to unknown tool:" "$merge_tool"
echo >&2 "Resetting to default..."
@@ -461,7 +461,7 @@ last_status=0
rollup_status=0
if test $# -eq 0 ; then
- files=`git ls-files -u | sed -e 's/^[^ ]* //' | sort -u`
+ files=$(git ls-files -u | sed -e 's/^[^ ]* //' | sort -u)
if test -z "$files" ; then
echo "No files need merging"
exit 0
--
1.6.1.3
^ permalink raw reply related [flat|nested] 19+ messages in thread
* [PATCH 3/8] sh-tools: add a git-sh-tools shell helper script
2009-03-30 5:03 ` [PATCH 2/8] mergetool: use $( ... ) instead of `backticks` David Aguilar
@ 2009-03-30 5:03 ` David Aguilar
2009-03-30 5:03 ` [PATCH 4/8] mergetool: refactor git-mergetool to use git-sh-tools David Aguilar
0 siblings, 1 reply; 19+ messages in thread
From: David Aguilar @ 2009-03-30 5:03 UTC (permalink / raw)
To: gitster; +Cc: git, David Aguilar
git-sh-tools holds functions common to the git-*tool commands.
Signed-off-by: David Aguilar <davvid@gmail.com>
---
.gitignore | 1 +
Documentation/git-sh-tools.txt | 49 +++++++++++++++++++++++++++++++++++++++
Makefile | 1 +
command-list.txt | 1 +
git-sh-tools.sh | 50 ++++++++++++++++++++++++++++++++++++++++
5 files changed, 102 insertions(+), 0 deletions(-)
create mode 100644 Documentation/git-sh-tools.txt
create mode 100644 git-sh-tools.sh
diff --git a/.gitignore b/.gitignore
index 966c886..cecf77e 100644
--- a/.gitignore
+++ b/.gitignore
@@ -114,6 +114,7 @@ git-rm
git-send-email
git-send-pack
git-sh-setup
+git-sh-tools
git-shell
git-shortlog
git-show
diff --git a/Documentation/git-sh-tools.txt b/Documentation/git-sh-tools.txt
new file mode 100644
index 0000000..055a10c
--- /dev/null
+++ b/Documentation/git-sh-tools.txt
@@ -0,0 +1,49 @@
+git-sh-tool(1)
+==============
+
+NAME
+----
+git-sh-tools - Common git *tool shell script functions
+
+SYNOPSIS
+--------
+'. "$(git --exec-path)/git-sh-tools"'
+
+DESCRIPTION
+-----------
+
+This is not a command the end user would want to run. Ever.
+This documentation is meant for people who are studying the
+Porcelain-ish scripts and/or are writing new ones.
+
+The 'git-sh-tools' scriptlet is designed to be sourced (using
+`.`) by other shell scripts to set up some functions for
+working with git merge/diff tools.
+
+Before sourcing it, your script should set up a few variables;
+`TOOL_MODE` is used to define the operation mode for various
+functions. 'diff' and 'merge' are valid values.
+
+FUNCTIONS
+---------
+valid_tool::
+ verifies that the specified merge tool is properly setup.
+
+valid_custom_tool::
+ verifies that a '(diff|merge)tool.<tool>.cmd' configuration exists.
+
+init_merge_tool_path::
+ sets up `$merge_tool_path` according to '(diff|merge)tool.<tool>.path'
+ configurations.
+
+Author
+------
+Written by David Aguilar <davvid@gmail.com>
+
+Documentation
+--------------
+Documentation by David Aguilar and the git-list <git@vger.kernel.org>.
+
+GIT
+---
+Part of the linkgit:git[1] suite
diff --git a/Makefile b/Makefile
index d77fd71..3b7c20f 100644
--- a/Makefile
+++ b/Makefile
@@ -292,6 +292,7 @@ SCRIPT_SH += git-rebase.sh
SCRIPT_SH += git-repack.sh
SCRIPT_SH += git-request-pull.sh
SCRIPT_SH += git-sh-setup.sh
+SCRIPT_SH += git-sh-tools.sh
SCRIPT_SH += git-stash.sh
SCRIPT_SH += git-submodule.sh
SCRIPT_SH += git-web--browse.sh
diff --git a/command-list.txt b/command-list.txt
index fb03a2e..c3b6c87 100644
--- a/command-list.txt
+++ b/command-list.txt
@@ -109,6 +109,7 @@ git-show-branch ancillaryinterrogators
git-show-index plumbinginterrogators
git-show-ref plumbinginterrogators
git-sh-setup purehelpers
+git-sh-tools purehelpers
git-stash mainporcelain
git-status mainporcelain common
git-stripspace purehelpers
diff --git a/git-sh-tools.sh b/git-sh-tools.sh
new file mode 100644
index 0000000..234bac7
--- /dev/null
+++ b/git-sh-tools.sh
@@ -0,0 +1,50 @@
+# Verifies that the chosen merge tool is properly setup.
+# Built-in merge tools are always valid.
+valid_tool() {
+ case "$1" in
+ kdiff3 | kompare | tkdiff | xxdiff | meld | opendiff | emerge | vimdiff | gvimdiff | ecmerge)
+ ;; # happy
+ *)
+ if ! valid_custom_tool "$1"; then
+ return 1
+ fi
+ ;;
+ esac
+}
+
+# Verifies that (difftool|mergetool).<tool>.cmd exists
+# Requires $TOOL_MODE to be set.
+valid_custom_tool() {
+ if test "$TOOL_MODE" = "diff"; then
+ merge_tool_cmd="$(git config difftool.$1.cmd)"
+ test -z "$merge_tool_cmd" &&
+ merge_tool_cmd="$(git config mergetool.$1.cmd)"
+ test -n "$merge_tool_cmd"
+ elif test "$TOOL_MODE" = "merge"; then
+ merge_tool_cmd="$(git config mergetool.$1.cmd)"
+ test -n "$merge_tool_cmd"
+ fi
+}
+
+
+# Set up $merge_tool_path for (diff|merge)tool.<tool>.path configurations
+init_merge_tool_path() {
+ if test "$TOOL_MODE" = "diff"; then
+ merge_tool_path=$(git config difftool."$1".path)
+ test -z "$merge_tool_path" &&
+ merge_tool_path=$(git config mergetool."$1".path)
+ elif test "$TOOL_MODE" = "merge"; then
+ merge_tool_path=$(git config mergetool."$1".path)
+ fi
+
+ if test -z "$merge_tool_path" ; then
+ case "$1" in
+ emerge)
+ merge_tool_path=emacs
+ ;;
+ *)
+ merge_tool_path=$1
+ ;;
+ esac
+ fi
+}
--
1.6.1.3
^ permalink raw reply related [flat|nested] 19+ messages in thread
* [PATCH 4/8] mergetool: refactor git-mergetool to use git-sh-tools
2009-03-30 5:03 ` [PATCH 3/8] sh-tools: add a git-sh-tools shell helper script David Aguilar
@ 2009-03-30 5:03 ` David Aguilar
2009-03-30 5:03 ` [PATCH 5/8] difftool: refactor git-difftool " David Aguilar
0 siblings, 1 reply; 19+ messages in thread
From: David Aguilar @ 2009-03-30 5:03 UTC (permalink / raw)
To: gitster; +Cc: git, David Aguilar
This removes the common valid_tool functions from git-mergetool.
Signed-off-by: David Aguilar <davvid@gmail.com>
---
git-mergetool.sh | 35 ++---------------------------------
1 files changed, 2 insertions(+), 33 deletions(-)
diff --git a/git-mergetool.sh b/git-mergetool.sh
index 5ea126c..ec20030 100755
--- a/git-mergetool.sh
+++ b/git-mergetool.sh
@@ -11,7 +11,9 @@
USAGE='[--tool=tool] [-y|--no-prompt|--prompt] [file to merge] ...'
SUBDIRECTORY_OK=Yes
OPTIONS_SPEC=
+TOOL_MODE=merge
. git-sh-setup
+. git-sh-tools
require_work_tree
# Returns true if the mode reflects a symlink
@@ -351,39 +353,6 @@ do
shift
done
-valid_custom_tool()
-{
- merge_tool_cmd="$(git config mergetool.$1.cmd)"
- test -n "$merge_tool_cmd"
-}
-
-valid_tool() {
- case "$1" in
- kdiff3 | tkdiff | xxdiff | meld | opendiff | \
- emerge | vimdiff | gvimdiff | ecmerge)
- ;; # happy
- *)
- if ! valid_custom_tool "$1"; then
- return 1
- fi
- ;;
- esac
-}
-
-init_merge_tool_path() {
- merge_tool_path=$(git config mergetool.$1.path)
- if test -z "$merge_tool_path" ; then
- case "$1" in
- emerge)
- merge_tool_path=emacs
- ;;
- *)
- merge_tool_path=$1
- ;;
- esac
- fi
-}
-
prompt_after_failed_merge() {
while true; do
printf "Continue merging other unresolved paths (y/n) ? "
--
1.6.1.3
^ permalink raw reply related [flat|nested] 19+ messages in thread
* [PATCH 5/8] difftool: refactor git-difftool to use git-sh-tools
2009-03-30 5:03 ` [PATCH 4/8] mergetool: refactor git-mergetool to use git-sh-tools David Aguilar
@ 2009-03-30 5:03 ` David Aguilar
2009-03-30 5:03 ` [PATCH 6/8] sh-tools: add a run_merge_tool function David Aguilar
0 siblings, 1 reply; 19+ messages in thread
From: David Aguilar @ 2009-03-30 5:03 UTC (permalink / raw)
To: gitster; +Cc: git, David Aguilar
This removes the common valid_tool functions from git-difftool.
Signed-off-by: David Aguilar <davvid@gmail.com>
---
git-difftool-helper.sh | 45 +++------------------------------------------
1 files changed, 3 insertions(+), 42 deletions(-)
diff --git a/git-difftool-helper.sh b/git-difftool-helper.sh
index 02bb135..2051a35 100755
--- a/git-difftool-helper.sh
+++ b/git-difftool-helper.sh
@@ -7,6 +7,9 @@
#
# Copyright (c) 2009 David Aguilar
+TOOL_MODE=diff
+. git-sh-tools
+
# difftool.prompt controls the default prompt/no-prompt behavior
# and is overridden with $GIT_DIFFTOOL*_PROMPT.
should_prompt () {
@@ -102,48 +105,6 @@ launch_merge_tool () {
esac
}
-# Verifies that (difftool|mergetool).<tool>.cmd exists
-valid_custom_tool() {
- merge_tool_cmd="$(git config difftool.$1.cmd)"
- test -z "$merge_tool_cmd" &&
- merge_tool_cmd="$(git config mergetool.$1.cmd)"
- test -n "$merge_tool_cmd"
-}
-
-# Verifies that the chosen merge tool is properly setup.
-# Built-in merge tools are always valid.
-valid_tool() {
- case "$1" in
- kdiff3 | kompare | tkdiff | xxdiff | meld | opendiff | emerge | vimdiff | gvimdiff | ecmerge)
- ;; # happy
- *)
- if ! valid_custom_tool "$1"
- then
- return 1
- fi
- ;;
- esac
-}
-
-# Sets up the merge_tool_path variable.
-# This handles the difftool.<tool>.path configuration.
-# This also falls back to mergetool defaults.
-init_merge_tool_path() {
- merge_tool_path=$(git config difftool."$1".path)
- test -z "$merge_tool_path" &&
- merge_tool_path=$(git config mergetool."$1".path)
- if test -z "$merge_tool_path"; then
- case "$1" in
- emerge)
- merge_tool_path=emacs
- ;;
- *)
- merge_tool_path="$1"
- ;;
- esac
- fi
-}
-
# Allow GIT_DIFF_TOOL and GIT_MERGE_TOOL to provide default values
test -n "$GIT_MERGE_TOOL" && merge_tool="$GIT_MERGE_TOOL"
test -n "$GIT_DIFF_TOOL" && merge_tool="$GIT_DIFF_TOOL"
--
1.6.1.3
^ permalink raw reply related [flat|nested] 19+ messages in thread
* [PATCH 6/8] sh-tools: add a run_merge_tool function
2009-03-30 5:03 ` [PATCH 5/8] difftool: refactor git-difftool " David Aguilar
@ 2009-03-30 5:03 ` David Aguilar
2009-03-30 5:03 ` [PATCH 7/8] mergetool: refactor git-mergetool to use run_merge_tool David Aguilar
` (2 more replies)
0 siblings, 3 replies; 19+ messages in thread
From: David Aguilar @ 2009-03-30 5:03 UTC (permalink / raw)
To: gitster; +Cc: git, David Aguilar
This function launches merge tools and will be used to refactor
git-(diff|merge)tool.
Signed-off-by: David Aguilar <davvid@gmail.com>
---
Documentation/git-sh-tools.txt | 3 +
git-sh-tools.sh | 147 +++++++++++++++++++++++++++++++++++++--
2 files changed, 142 insertions(+), 8 deletions(-)
diff --git a/Documentation/git-sh-tools.txt b/Documentation/git-sh-tools.txt
index 055a10c..68d1b37 100644
--- a/Documentation/git-sh-tools.txt
+++ b/Documentation/git-sh-tools.txt
@@ -36,6 +36,9 @@ init_merge_tool_path::
sets up `$merge_tool_path` according to '(diff|merge)tool.<tool>.path'
configurations.
+run_merge_tool::
+ runs the specified merge tool.
+
Author
------
Written by David Aguilar <davvid@gmail.com>
diff --git a/git-sh-tools.sh b/git-sh-tools.sh
index 234bac7..5c7bd18 100644
--- a/git-sh-tools.sh
+++ b/git-sh-tools.sh
@@ -12,29 +12,34 @@ valid_tool() {
esac
}
+# Test whether we're in merge mode
+mergetool_mode()
+{
+ test "$TOOL_MODE" = "merge"
+}
+
# Verifies that (difftool|mergetool).<tool>.cmd exists
# Requires $TOOL_MODE to be set.
valid_custom_tool() {
- if test "$TOOL_MODE" = "diff"; then
- merge_tool_cmd="$(git config difftool.$1.cmd)"
- test -z "$merge_tool_cmd" &&
+ if mergetool_mode; then
merge_tool_cmd="$(git config mergetool.$1.cmd)"
test -n "$merge_tool_cmd"
- elif test "$TOOL_MODE" = "merge"; then
+ else
+ merge_tool_cmd="$(git config difftool.$1.cmd)"
+ test -z "$merge_tool_cmd" &&
merge_tool_cmd="$(git config mergetool.$1.cmd)"
test -n "$merge_tool_cmd"
fi
}
-
# Set up $merge_tool_path for (diff|merge)tool.<tool>.path configurations
init_merge_tool_path() {
- if test "$TOOL_MODE" = "diff"; then
+ if mergetool_mode; then
+ merge_tool_path=$(git config mergetool."$1".path)
+ else
merge_tool_path=$(git config difftool."$1".path)
test -z "$merge_tool_path" &&
merge_tool_path=$(git config mergetool."$1".path)
- elif test "$TOOL_MODE" = "merge"; then
- merge_tool_path=$(git config mergetool."$1".path)
fi
if test -z "$merge_tool_path" ; then
@@ -48,3 +53,129 @@ init_merge_tool_path() {
esac
fi
}
+
+# Runs a side-by-side merge tool
+run_merge_tool()
+{
+ merge_tool="$1"
+
+ # base_present is always false when !mergetool_mode
+ case "$merge_tool" in
+ kdiff3)
+ if mergetool_mode; then
+ base=Baes
+ local=Local
+ remote=Remote
+ else
+ base=A
+ local=A
+ remote=B
+ fi
+ 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=$?
+ ;;
+ tkdiff)
+ if base_present; then
+ "$merge_tool_path" -a "$BASE" -o "$MERGED" "$LOCAL" "$REMOTE"
+ else
+ "$merge_tool_path" -o "$MERGED" "$LOCAL" "$REMOTE"
+ fi
+ status=$?
+ ;;
+ meld)
+ mergetool_mode && touch "$BACKUP"
+ "$merge_tool_path" "$LOCAL" "$MERGED" "$REMOTE"
+ mergetool_mode && check_unchanged
+ ;;
+ vimdiff)
+ mergetool_mode && touch "$BACKUP"
+ "$merge_tool_path" -c "wincmd l" "$LOCAL" "$MERGED" "$REMOTE"
+ mergetool_mode && check_unchanged
+ ;;
+ gvimdiff)
+ mergetool_mode && touch "$BACKUP"
+ "$merge_tool_path" -c "wincmd l" -f "$LOCAL" "$MERGED" "$REMOTE"
+ mergetool_mode && check_unchanged
+ ;;
+ xxdiff)
+ if mergetool_mode; then
+ touch "$BACKUP"
+ xtra_args='--show-merged-pane'
+ else
+ xtra_args=
+ fi
+ if base_present; then
+ "$merge_tool_path" -X $xtra_args \
+ -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 $xtra_args \
+ -R 'Accel.SaveAsMerged: "Ctrl-S"' \
+ -R 'Accel.Search: "Ctrl+F"' \
+ -R 'Accel.SearchForward: "Ctrl-G"' \
+ --merged-file "$MERGED" "$LOCAL" "$REMOTE"
+ fi
+ mergetool_mode && check_unchanged
+ ;;
+ opendiff)
+ mergetool_mode && 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
+ mergetool_mode && check_unchanged
+ ;;
+ ecmerge)
+ mergetool_mode && 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
+ mergetool_mode && check_unchanged
+ ;;
+ emerge)
+ 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=$?
+ ;;
+ *)
+ if test -n "$merge_tool_cmd"; then
+ if test "$merge_tool_trust_exit_code" = "false"; then
+ mergetool_mode && touch "$BACKUP"
+ ( eval $merge_tool_cmd )
+ mergetool_mode && check_unchanged
+ else
+ ( eval $merge_tool_cmd )
+ status=$?
+ fi
+ fi
+ ;;
+ esac
+ return $status
+}
--
1.6.1.3
^ permalink raw reply related [flat|nested] 19+ messages in thread
* [PATCH 7/8] mergetool: refactor git-mergetool to use run_merge_tool
2009-03-30 5:03 ` [PATCH 6/8] sh-tools: add a run_merge_tool function David Aguilar
@ 2009-03-30 5:03 ` David Aguilar
2009-03-30 5:03 ` [PATCH 8/8] difftool: refactor git-difftool-helper " David Aguilar
2009-03-30 6:55 ` [PATCH 6/8] sh-tools: add a run_merge_tool function Markus Heidelberg
2009-03-30 7:32 ` Markus Heidelberg
2 siblings, 1 reply; 19+ messages in thread
From: David Aguilar @ 2009-03-30 5:03 UTC (permalink / raw)
To: gitster; +Cc: git, David Aguilar
This removes most of the duplicated code in git-mergetool.
Signed-off-by: David Aguilar <davvid@gmail.com>
---
git-mergetool.sh | 106 ++----------------------------------------------------
1 files changed, 3 insertions(+), 103 deletions(-)
diff --git a/git-mergetool.sh b/git-mergetool.sh
index ec20030..0f5033c 100755
--- a/git-mergetool.sh
+++ b/git-mergetool.sh
@@ -191,109 +191,9 @@ merge_file () {
read ans
fi
- case "$merge_tool" in
- kdiff3)
- 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=$?
- ;;
- tkdiff)
- if base_present ; then
- "$merge_tool_path" -a "$BASE" -o "$MERGED" "$LOCAL" "$REMOTE"
- else
- "$merge_tool_path" -o "$MERGED" "$LOCAL" "$REMOTE"
- fi
- status=$?
- ;;
- meld)
- touch "$BACKUP"
- "$merge_tool_path" "$LOCAL" "$MERGED" "$REMOTE"
- check_unchanged
- ;;
- vimdiff)
- touch "$BACKUP"
- "$merge_tool_path" -c "wincmd l" "$LOCAL" "$MERGED" "$REMOTE"
- check_unchanged
- ;;
- gvimdiff)
- touch "$BACKUP"
- "$merge_tool_path" -c "wincmd l" -f "$LOCAL" "$MERGED" "$REMOTE"
- check_unchanged
- ;;
- xxdiff)
- 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 --show-merged-pane \
- -R 'Accel.SaveAsMerged: "Ctrl-S"' \
- -R 'Accel.Search: "Ctrl+F"' \
- -R 'Accel.SearchForward: "Ctrl-G"' \
- --merged-file "$MERGED" "$LOCAL" "$REMOTE"
- fi
- check_unchanged
- ;;
- opendiff)
- 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
- ;;
- ecmerge)
- 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
- ;;
- emerge)
- 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=$?
- ;;
- *)
- if test -n "$merge_tool_cmd"; then
- if test "$merge_tool_trust_exit_code" = "false"; then
- touch "$BACKUP"
- ( eval $merge_tool_cmd )
- check_unchanged
- else
- ( eval $merge_tool_cmd )
- status=$?
- fi
- fi
- ;;
- esac
+ run_merge_tool "$merge_tool"
+ status=$?
+
if test "$status" -ne 0; then
echo "merge of $MERGED failed" 1>&2
mv -- "$BACKUP" "$MERGED"
--
1.6.1.3
^ permalink raw reply related [flat|nested] 19+ messages in thread
* [PATCH 8/8] difftool: refactor git-difftool-helper to use run_merge_tool
2009-03-30 5:03 ` [PATCH 7/8] mergetool: refactor git-mergetool to use run_merge_tool David Aguilar
@ 2009-03-30 5:03 ` David Aguilar
0 siblings, 0 replies; 19+ messages in thread
From: David Aguilar @ 2009-03-30 5:03 UTC (permalink / raw)
To: gitster; +Cc: git, David Aguilar
This removes most of the duplicated code in git-difftool-helper.
Signed-off-by: David Aguilar <davvid@gmail.com>
---
git-difftool-helper.sh | 67 ++++-------------------------------------------
1 files changed, 6 insertions(+), 61 deletions(-)
diff --git a/git-difftool-helper.sh b/git-difftool-helper.sh
index 2051a35..d90277b 100755
--- a/git-difftool-helper.sh
+++ b/git-difftool-helper.sh
@@ -10,6 +10,11 @@
TOOL_MODE=diff
. git-sh-tools
+base_present()
+{
+ return 1
+}
+
# difftool.prompt controls the default prompt/no-prompt behavior
# and is overridden with $GIT_DIFFTOOL*_PROMPT.
should_prompt () {
@@ -42,67 +47,7 @@ launch_merge_tool () {
fi
# Run the appropriate merge tool command
- case "$merge_tool" in
- kdiff3)
- basename=$(basename "$MERGED")
- "$merge_tool_path" --auto \
- --L1 "$basename (A)" \
- --L2 "$basename (B)" \
- -o "$MERGED" "$LOCAL" "$REMOTE" \
- > /dev/null 2>&1
- ;;
-
- kompare)
- "$merge_tool_path" "$LOCAL" "$REMOTE"
- ;;
-
- tkdiff)
- "$merge_tool_path" -o "$MERGED" "$LOCAL" "$REMOTE"
- ;;
-
- meld)
- "$merge_tool_path" "$LOCAL" "$REMOTE"
- ;;
-
- vimdiff)
- "$merge_tool_path" -c "wincmd l" "$LOCAL" "$REMOTE"
- ;;
-
- gvimdiff)
- "$merge_tool_path" -c "wincmd l" -f "$LOCAL" "$REMOTE"
- ;;
-
- xxdiff)
- "$merge_tool_path" \
- -X \
- -R 'Accel.SaveAsMerged: "Ctrl-S"' \
- -R 'Accel.Search: "Ctrl+F"' \
- -R 'Accel.SearchForward: "Ctrl-G"' \
- --merged-file "$MERGED" \
- "$LOCAL" "$REMOTE"
- ;;
-
- opendiff)
- "$merge_tool_path" "$LOCAL" "$REMOTE" \
- -merge "$MERGED" | cat
- ;;
-
- ecmerge)
- "$merge_tool_path" "$LOCAL" "$REMOTE" \
- --default --mode=merge2 --to="$MERGED"
- ;;
-
- emerge)
- "$merge_tool_path" -f emerge-files-command \
- "$LOCAL" "$REMOTE" "$(basename "$MERGED")"
- ;;
-
- *)
- if test -n "$merge_tool_cmd"; then
- ( eval $merge_tool_cmd )
- fi
- ;;
- esac
+ run_merge_tool "$merge_tool"
}
# Allow GIT_DIFF_TOOL and GIT_MERGE_TOOL to provide default values
--
1.6.1.3
^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [PATCH 6/8] sh-tools: add a run_merge_tool function
2009-03-30 5:03 ` [PATCH 6/8] sh-tools: add a run_merge_tool function David Aguilar
2009-03-30 5:03 ` [PATCH 7/8] mergetool: refactor git-mergetool to use run_merge_tool David Aguilar
@ 2009-03-30 6:55 ` Markus Heidelberg
2009-03-30 7:32 ` Markus Heidelberg
2 siblings, 0 replies; 19+ messages in thread
From: Markus Heidelberg @ 2009-03-30 6:55 UTC (permalink / raw)
To: David Aguilar; +Cc: gitster, git
David Aguilar, 30.03.2009:
> + if mergetool_mode; then
> + base=Baes
Base
> + local=Local
> + remote=Remote
> + else
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re:
2009-03-30 5:03 (unknown), David Aguilar
2009-03-30 5:03 ` [PATCH 1/8] mergetool: use tabs consistently David Aguilar
@ 2009-03-30 7:02 ` Markus Heidelberg
2009-03-30 8:46 ` Re: Junio C Hamano
1 sibling, 1 reply; 19+ messages in thread
From: Markus Heidelberg @ 2009-03-30 7:02 UTC (permalink / raw)
To: David Aguilar; +Cc: gitster, git
David Aguilar, 30.03.2009:
> This is based on top of Junio's "pu" branch and is a
> continuation of the recent difftool series.
For everyone who wants to apply the patch series: Patch 5/8 depends on
this:
[PATCH v2] difftool: add support for a difftool.prompt config variable
sent about 8 minutes before this series.
Markus
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 6/8] sh-tools: add a run_merge_tool function
2009-03-30 5:03 ` [PATCH 6/8] sh-tools: add a run_merge_tool function David Aguilar
2009-03-30 5:03 ` [PATCH 7/8] mergetool: refactor git-mergetool to use run_merge_tool David Aguilar
2009-03-30 6:55 ` [PATCH 6/8] sh-tools: add a run_merge_tool function Markus Heidelberg
@ 2009-03-30 7:32 ` Markus Heidelberg
2009-03-30 7:46 ` David Aguilar
2 siblings, 1 reply; 19+ messages in thread
From: Markus Heidelberg @ 2009-03-30 7:32 UTC (permalink / raw)
To: David Aguilar; +Cc: gitster, git
David Aguilar, 30.03.2009:
> +# Runs a side-by-side merge tool
> +run_merge_tool()
> +{
> + merge_tool="$1"
> +
> + # base_present is always false when !mergetool_mode
> + case "$merge_tool" in
> + kdiff3)
> [...]
Kompare is missing here. Note, that this is only diff tool, not for
merges.
Markus
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 6/8] sh-tools: add a run_merge_tool function
2009-03-30 7:32 ` Markus Heidelberg
@ 2009-03-30 7:46 ` David Aguilar
0 siblings, 0 replies; 19+ messages in thread
From: David Aguilar @ 2009-03-30 7:46 UTC (permalink / raw)
To: Markus Heidelberg; +Cc: gitster, git
On 0, Markus Heidelberg <markus.heidelberg@web.de> wrote:
> David Aguilar, 30.03.2009:
> > +# Runs a side-by-side merge tool
> > +run_merge_tool()
> > +{
> > + merge_tool="$1"
> > +
> > + # base_present is always false when !mergetool_mode
> > + case "$merge_tool" in
> > + kdiff3)
> > [...]
>
> Kompare is missing here. Note, that this is only diff tool, not for
> merges.
>
> Markus
>
Thanks, will fixup asap.
I botched the --cover letter too ;)
note to self: no more refactoring while sleepy on plane.
--
David
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 1/8] mergetool: use tabs consistently
2009-03-30 5:03 ` [PATCH 1/8] mergetool: use tabs consistently David Aguilar
2009-03-30 5:03 ` [PATCH 2/8] mergetool: use $( ... ) instead of `backticks` David Aguilar
@ 2009-03-30 8:44 ` Junio C Hamano
2009-03-30 9:22 ` David Aguilar
2009-03-30 21:35 ` Charles Bailey
1 sibling, 2 replies; 19+ messages in thread
From: Junio C Hamano @ 2009-03-30 8:44 UTC (permalink / raw)
To: David Aguilar; +Cc: git, Charles Bailey
Thanks.
Even though this [1/8] is obviously regression free, and I think the
overall direction in which the series is going is good, I'll wait until I
hear Acks from Charles Bailey for the parts that involve mergetool. I do
not use either mergetool nor difftool myself, and going over every single
line of this series to spot potential regression is beyond my bandwidth
right now.
I do not think bits only common between mergetool and difftool should be
called with a very generic name "sh-tools". We didn't call the result of
a similar refactoring for launching web browser from help and instaweb
context with such a generic name (it is called git-web--browse).
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re:
2009-03-30 7:02 ` Markus Heidelberg
@ 2009-03-30 8:46 ` Junio C Hamano
0 siblings, 0 replies; 19+ messages in thread
From: Junio C Hamano @ 2009-03-30 8:46 UTC (permalink / raw)
To: markus.heidelberg; +Cc: David Aguilar, git
Markus Heidelberg <markus.heidelberg@web.de> writes:
> David Aguilar, 30.03.2009:
>> This is based on top of Junio's "pu" branch and is a
>> continuation of the recent difftool series.
>
> For everyone who wants to apply the patch series: Patch 5/8 depends on
> this:
> [PATCH v2] difftool: add support for a difftool.prompt config variable
> sent about 8 minutes before this series.
Thanks for keeping an eye on this series, as a recent contributor to
mergetool. I do not use mergetool myself, but this refactoring seems to
be a good idea in general, and help in reviewing the series is very much
appreciated.
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 1/8] mergetool: use tabs consistently
2009-03-30 8:44 ` [PATCH 1/8] mergetool: use tabs consistently Junio C Hamano
@ 2009-03-30 9:22 ` David Aguilar
2009-03-30 21:35 ` Charles Bailey
1 sibling, 0 replies; 19+ messages in thread
From: David Aguilar @ 2009-03-30 9:22 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Charles Bailey
On 0, Junio C Hamano <gitster@pobox.com> wrote:
> Thanks.
>
> Even though this [1/8] is obviously regression free, and I think the
> overall direction in which the series is going is good, I'll wait until I
> hear Acks from Charles Bailey for the parts that involve mergetool. I do
> not use either mergetool nor difftool myself, and going over every single
> line of this series to spot potential regression is beyond my bandwidth
> right now.
>
> I do not think bits only common between mergetool and difftool should be
> called with a very generic name "sh-tools". We didn't call the result of
> a similar refactoring for launching web browser from help and instaweb
> context with such a generic name (it is called git-web--browse).
I also felt iffy about the name.
Maybe...
git-interactive--tools ?
git-merge-diff--tools ?
git-mergetool--lib ?
(i'm pretty bad with this naming stuff ;))
(I have --interactive on my brain thanks to Ping's patch ;))
I can rebase as needed once we get more feedback, particularly
notes from Charles.
--
David
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 1/8] mergetool: use tabs consistently
2009-03-30 8:44 ` [PATCH 1/8] mergetool: use tabs consistently Junio C Hamano
2009-03-30 9:22 ` David Aguilar
@ 2009-03-30 21:35 ` Charles Bailey
2009-03-31 6:36 ` David Aguilar
1 sibling, 1 reply; 19+ messages in thread
From: Charles Bailey @ 2009-03-30 21:35 UTC (permalink / raw)
To: Junio C Hamano; +Cc: David Aguilar, git
On Mon, Mar 30, 2009 at 01:44:01AM -0700, Junio C Hamano wrote:
> Thanks.
>
> Even though this [1/8] is obviously regression free, and I think the
> overall direction in which the series is going is good, I'll wait until I
> hear Acks from Charles Bailey for the parts that involve mergetool. I do
> not use either mergetool nor difftool myself, and going over every single
> line of this series to spot potential regression is beyond my bandwidth
> right now.
>
> I do not think bits only common between mergetool and difftool should be
> called with a very generic name "sh-tools". We didn't call the result of
> a similar refactoring for launching web browser from help and instaweb
> context with such a generic name (it is called git-web--browse).
OK, I've just had a very quick review of the patch series and, in
general, I like it.
I don't much like [1/8] though. I'm all in favour of consistency, but
this patch touches most of the lines in git-mergetool and tries to go
the opposite way to the consistency drive that we were trying to
introduce gradually (i.e. only through lines materially affected by
subsequent patches) in:
commit 0eea345111a9b9fea4dd2841b80bc7d62964e812
Author: Charles Bailey <charles@hashpling.org>
Date: Thu Nov 13 12:41:13 2008 +0000
Fix some tab/space inconsistencies in git-mergetool.sh
If you'd gone the other way the patch to consistency would only affect
23 lines rather than 347 lines and all bar 3 of these lines you
subsequently remove from git-mergetool.sh in later patches anyway.
[2/8] - looks good.
[3/8] - no mergetool impact.
[4/8] - Hmmm, OK. Even so at this point, I'm getting slightly iffy
feelings about the whole init_merge_tool_path sets a variable needed
by the calling script. I know it's only scripting and not programming,
but it seemed less bad to set (global) variables in sh functions when
they were all in the same sh script.
[5/8] - no mergtool impact.
[6/8] - ditto
[7/8] - OK, here's where my uneasiness about global script variables
vs. parameters really gets going. Why is merge_tool a parameter when
it's setup once and doesn't change in the invocation of a script, yet
base_present is a script global but can vary between sets of paths to
be merged?
I fully appreciate that this is just inheriting the way things are
and that they weren't beautiful before, but it somehow seems even
worse when the variables are set in one script and used from a
function in a separate sourced script. We're definitely setting up a
very strong coupling between the two scripts which will make it harder
to change either in the future.
[8/8] - no mergetool impact here.
On the plus side, I really like the introduction and function of the
run_mergetool function. It's exactly the split that will make
extending mergetool resolves of file vs. symlink vs. directory easier
in the future. I have a similar split in some slow brewing patches
myself.
I think that [1/8] is the only patch that I'd relucatant to ack, as it
seems like unnecessary churn and change of direction. Here's a sample
patch for consistency 'the other way'. As I mentioned before, the
first to hunks are made redundant by your subsequent changes anyway,
so I only counted 3 lines that are currently inconsistent in
git-mergetool as it stands at the moment.
Sample patch fixing consistent whitespace 'the other way'.
---
git-mergetool.sh | 46 +++++++++++++++++++++++-----------------------
1 files changed, 23 insertions(+), 23 deletions(-)
diff --git a/git-mergetool.sh b/git-mergetool.sh
index 87fa88a..1588b5f 100755
--- a/git-mergetool.sh
+++ b/git-mergetool.sh
@@ -344,29 +344,29 @@ valid_custom_tool()
}
valid_tool() {
- case "$1" in
- kdiff3 | tkdiff | xxdiff | meld | opendiff | emerge | vimdiff | gvimdiff | ecmerge)
- ;; # happy
- *)
- if ! valid_custom_tool "$1"; then
- return 1
- fi
- ;;
- esac
+ case "$1" in
+ kdiff3 | tkdiff | xxdiff | meld | opendiff | emerge | vimdiff | gvimdiff | ecmerge)
+ ;; # happy
+ *)
+ if ! valid_custom_tool "$1"; then
+ return 1
+ fi
+ ;;
+ esac
}
init_merge_tool_path() {
- merge_tool_path=`git config mergetool.$1.path`
- if test -z "$merge_tool_path" ; then
- case "$1" in
- emerge)
- merge_tool_path=emacs
- ;;
- *)
- merge_tool_path=$1
- ;;
- esac
- fi
+ merge_tool_path=`git config mergetool.$1.path`
+ if test -z "$merge_tool_path" ; then
+ case "$1" in
+ emerge)
+ merge_tool_path=emacs
+ ;;
+ *)
+ merge_tool_path=$1
+ ;;
+ esac
+ fi
}
prompt_after_failed_merge() {
@@ -389,9 +389,9 @@ prompt_after_failed_merge() {
if test -z "$merge_tool"; then
merge_tool=`git config merge.tool`
if test -n "$merge_tool" && ! valid_tool "$merge_tool"; then
- echo >&2 "git config option merge.tool set to unknown tool: $merge_tool"
- echo >&2 "Resetting to default..."
- unset merge_tool
+ echo >&2 "git config option merge.tool set to unknown tool: $merge_tool"
+ echo >&2 "Resetting to default..."
+ unset merge_tool
fi
fi
--
1.6.2.323.geaf6e
--
Charles Bailey
http://ccgi.hashpling.plus.com/blog/
^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [PATCH 1/8] mergetool: use tabs consistently
2009-03-30 21:35 ` Charles Bailey
@ 2009-03-31 6:36 ` David Aguilar
2009-04-01 17:56 ` Junio C Hamano
0 siblings, 1 reply; 19+ messages in thread
From: David Aguilar @ 2009-03-31 6:36 UTC (permalink / raw)
To: Charles Bailey; +Cc: Junio C Hamano, git
On 0, Charles Bailey <charles@hashpling.org> wrote:
> On Mon, Mar 30, 2009 at 01:44:01AM -0700, Junio C Hamano wrote:
>
> I don't much like [1/8] though. I'm all in favour of consistency, but
> this patch touches most of the lines in git-mergetool and tries to go
> the opposite way to the consistency drive that we were trying to
> introduce gradually (i.e. only through lines materially affected by
> subsequent patches) in:
Sounds good. I'll re-roll and give the refactoring another go.
I think we can definitely do better, and more importantly, I
think we can decouple things by using less globals.
Junio, did you have any comments about patch v2
"difftool: add support for difftool.prompt config variable"?
This series was based on top of that patch so I'm wondering if I
should do that again.
>
> commit 0eea345111a9b9fea4dd2841b80bc7d62964e812
> Author: Charles Bailey <charles@hashpling.org>
> Date: Thu Nov 13 12:41:13 2008 +0000
>
> Fix some tab/space inconsistencies in git-mergetool.sh
>
> If you'd gone the other way the patch to consistency would only affect
> 23 lines rather than 347 lines and all bar 3 of these lines you
> subsequently remove from git-mergetool.sh in later patches anyway.
>
> [2/8] - looks good.
>
> [3/8] - no mergetool impact.
>
> [4/8] - Hmmm, OK. Even so at this point, I'm getting slightly iffy
> feelings about the whole init_merge_tool_path sets a variable needed
> by the calling script. I know it's only scripting and not programming,
> but it seemed less bad to set (global) variables in sh functions when
> they were all in the same sh script.
>
> [5/8] - no mergtool impact.
>
> [6/8] - ditto
>
> [7/8] - OK, here's where my uneasiness about global script variables
> vs. parameters really gets going. Why is merge_tool a parameter when
> it's setup once and doesn't change in the invocation of a script, yet
> base_present is a script global but can vary between sets of paths to
> be merged?
>
> I fully appreciate that this is just inheriting the way things are
> and that they weren't beautiful before, but it somehow seems even
> worse when the variables are set in one script and used from a
> function in a separate sourced script. We're definitely setting up a
> very strong coupling between the two scripts which will make it harder
> to change either in the future.
>
> [8/8] - no mergetool impact here.
>
> On the plus side, I really like the introduction and function of the
> run_mergetool function. It's exactly the split that will make
> extending mergetool resolves of file vs. symlink vs. directory easier
> in the future. I have a similar split in some slow brewing patches
> myself.
>
> I think that [1/8] is the only patch that I'd relucatant to ack, as it
> seems like unnecessary churn and change of direction. Here's a sample
> patch for consistency 'the other way'. As I mentioned before, the
> first to hunks are made redundant by your subsequent changes anyway,
> so I only counted 3 lines that are currently inconsistent in
> git-mergetool as it stands at the moment.
>
> Sample patch fixing consistent whitespace 'the other way'.
> ---
> git-mergetool.sh | 46 +++++++++++++++++++++++-----------------------
> 1 files changed, 23 insertions(+), 23 deletions(-)
>
> diff --git a/git-mergetool.sh b/git-mergetool.sh
> index 87fa88a..1588b5f 100755
> --- a/git-mergetool.sh
> +++ b/git-mergetool.sh
> @@ -344,29 +344,29 @@ valid_custom_tool()
> }
>
> valid_tool() {
> - case "$1" in
> - kdiff3 | tkdiff | xxdiff | meld | opendiff | emerge | vimdiff | gvimdiff | ecmerge)
> - ;; # happy
> - *)
> - if ! valid_custom_tool "$1"; then
> - return 1
> - fi
> - ;;
> - esac
> + case "$1" in
> + kdiff3 | tkdiff | xxdiff | meld | opendiff | emerge | vimdiff | gvimdiff | ecmerge)
> + ;; # happy
> + *)
> + if ! valid_custom_tool "$1"; then
> + return 1
> + fi
> + ;;
> + esac
> }
>
> init_merge_tool_path() {
> - merge_tool_path=`git config mergetool.$1.path`
> - if test -z "$merge_tool_path" ; then
> - case "$1" in
> - emerge)
> - merge_tool_path=emacs
> - ;;
> - *)
> - merge_tool_path=$1
> - ;;
> - esac
> - fi
> + merge_tool_path=`git config mergetool.$1.path`
> + if test -z "$merge_tool_path" ; then
> + case "$1" in
> + emerge)
> + merge_tool_path=emacs
> + ;;
> + *)
> + merge_tool_path=$1
> + ;;
> + esac
> + fi
> }
>
> prompt_after_failed_merge() {
> @@ -389,9 +389,9 @@ prompt_after_failed_merge() {
> if test -z "$merge_tool"; then
> merge_tool=`git config merge.tool`
> if test -n "$merge_tool" && ! valid_tool "$merge_tool"; then
> - echo >&2 "git config option merge.tool set to unknown tool: $merge_tool"
> - echo >&2 "Resetting to default..."
> - unset merge_tool
> + echo >&2 "git config option merge.tool set to unknown tool: $merge_tool"
> + echo >&2 "Resetting to default..."
> + unset merge_tool
> fi
> fi
>
> --
> 1.6.2.323.geaf6e
>
> --
> Charles Bailey
> http://ccgi.hashpling.plus.com/blog/
--
David
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 1/8] mergetool: use tabs consistently
2009-03-31 6:36 ` David Aguilar
@ 2009-04-01 17:56 ` Junio C Hamano
0 siblings, 0 replies; 19+ messages in thread
From: Junio C Hamano @ 2009-04-01 17:56 UTC (permalink / raw)
To: David Aguilar; +Cc: Charles Bailey, git
David Aguilar <davvid@gmail.com> writes:
> On 0, Charles Bailey <charles@hashpling.org> wrote:
>> On Mon, Mar 30, 2009 at 01:44:01AM -0700, Junio C Hamano wrote:
>>
>> I don't much like [1/8] though. I'm all in favour of consistency, but
>> this patch touches most of the lines in git-mergetool and tries to go
>> the opposite way to the consistency drive that we were trying to
>> introduce gradually (i.e. only through lines materially affected by
>> subsequent patches) in:
>
> Sounds good. I'll re-roll and give the refactoring another go.
>
> I think we can definitely do better, and more importantly, I
> think we can decouple things by using less globals.
>
> Junio, did you have any comments about patch v2
> "difftool: add support for difftool.prompt config variable"?
I only took a cursory look but it; you did seem to have based it on the
one I queued in 'pu' with a typo-fixup and it looked fine.
Thanks.
^ permalink raw reply [flat|nested] 19+ messages in thread
end of thread, other threads:[~2009-04-01 17:58 UTC | newest]
Thread overview: 19+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2009-03-30 5:03 (unknown), David Aguilar
2009-03-30 5:03 ` [PATCH 1/8] mergetool: use tabs consistently David Aguilar
2009-03-30 5:03 ` [PATCH 2/8] mergetool: use $( ... ) instead of `backticks` David Aguilar
2009-03-30 5:03 ` [PATCH 3/8] sh-tools: add a git-sh-tools shell helper script David Aguilar
2009-03-30 5:03 ` [PATCH 4/8] mergetool: refactor git-mergetool to use git-sh-tools David Aguilar
2009-03-30 5:03 ` [PATCH 5/8] difftool: refactor git-difftool " David Aguilar
2009-03-30 5:03 ` [PATCH 6/8] sh-tools: add a run_merge_tool function David Aguilar
2009-03-30 5:03 ` [PATCH 7/8] mergetool: refactor git-mergetool to use run_merge_tool David Aguilar
2009-03-30 5:03 ` [PATCH 8/8] difftool: refactor git-difftool-helper " David Aguilar
2009-03-30 6:55 ` [PATCH 6/8] sh-tools: add a run_merge_tool function Markus Heidelberg
2009-03-30 7:32 ` Markus Heidelberg
2009-03-30 7:46 ` David Aguilar
2009-03-30 8:44 ` [PATCH 1/8] mergetool: use tabs consistently Junio C Hamano
2009-03-30 9:22 ` David Aguilar
2009-03-30 21:35 ` Charles Bailey
2009-03-31 6:36 ` David Aguilar
2009-04-01 17:56 ` Junio C Hamano
2009-03-30 7:02 ` Markus Heidelberg
2009-03-30 8:46 ` Re: Junio C Hamano
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
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 read-only IMAP folder(s) and NNTP newsgroup(s).