* git-{diff,merge} refactor round 2
@ 2009-04-01 12:55 David Aguilar
2009-04-01 12:55 ` [PATCH 01/10] difftool: add support for a difftool.prompt config variable David Aguilar
` (2 more replies)
0 siblings, 3 replies; 26+ messages in thread
From: David Aguilar @ 2009-04-01 12:55 UTC (permalink / raw)
To: gitster, charles; +Cc: git
Here's the 2nd round of refactoring.
This is based on Junio's pu branch.
I'm including the difftool.prompt patch in this series
because it is a dependency and including it here makes
that obvious.
I tried to keep the dependencies untangled while still being
able to manage the various command-line flags all in one
place. Alas, this is shell so it can only be so elegant.
I went ahead and rolled in the "remove -o $MERGED" from
tkdiff for the diff mode case.
Still TODO:
incorporate the "add diffuse as a merge tool" patch.
Is there more that can be refactored?
Probably the part that sets up candidate mergetools,
replacing that with a function might be useful.
Here's what I've got so far.
^ permalink raw reply [flat|nested] 26+ messages in thread* [PATCH 01/10] difftool: add support for a difftool.prompt config variable 2009-04-01 12:55 git-{diff,merge} refactor round 2 David Aguilar @ 2009-04-01 12:55 ` David Aguilar 2009-04-01 12:55 ` [PATCH 02/10] mergetool: use $( ... ) instead of `backticks` David Aguilar 2009-04-02 19:59 ` git-{diff,merge} refactor round 2 Charles Bailey 2009-04-05 2:58 ` Markus Heidelberg 2 siblings, 1 reply; 26+ messages in thread From: David Aguilar @ 2009-04-01 12:55 UTC (permalink / raw) To: gitster, charles; +Cc: git, David Aguilar difftool now supports difftool.prompt so that users do not have to pass --no-prompt or hit enter each time a diff tool is launched. The --prompt flag overrides the configuration variable. Signed-off-by: David Aguilar <davvid@gmail.com> --- Documentation/config.txt | 3 ++ Documentation/git-difftool.txt | 10 +++++- git-difftool-helper.sh | 10 +++++- git-difftool.perl | 15 +++++++-- t/t7800-difftool.sh | 64 ++++++++++++++++++++++++++++++++++++++++ 5 files changed, 96 insertions(+), 6 deletions(-) diff --git a/Documentation/config.txt b/Documentation/config.txt index 185a9ef..79c8b66 100644 --- a/Documentation/config.txt +++ b/Documentation/config.txt @@ -684,6 +684,9 @@ difftool.<tool>.cmd:: is set to the name of the temporary file containing the contents of the diff post-image. +difftool.prompt:: + Prompt before each invocation of the diff tool. + diff.wordRegex:: A POSIX Extended Regular Expression used to determine what is a "word" when performing word-by-word difference calculations. Character diff --git a/Documentation/git-difftool.txt b/Documentation/git-difftool.txt index a00e943..73d4782 100644 --- a/Documentation/git-difftool.txt +++ b/Documentation/git-difftool.txt @@ -7,7 +7,7 @@ git-difftool - Show changes using common diff tools SYNOPSIS -------- -'git difftool' [--tool=<tool>] [-y|--no-prompt] [<'git diff' options>] +'git difftool' [--tool=<tool>] [-y|--no-prompt|--prompt] [<'git diff' options>] DESCRIPTION ----------- @@ -21,6 +21,11 @@ OPTIONS --no-prompt:: Do not prompt before launching a diff tool. +--prompt:: + Prompt before each invocation of the diff tool. + This is the default behaviour; the option is provided to + override any configuration settings. + -t <tool>:: --tool=<tool>:: Use the diff tool specified by <tool>. @@ -72,6 +77,9 @@ difftool.<tool>.cmd:: + See the `--tool=<tool>` option above for more details. +difftool.prompt:: + Prompt before each invocation of the diff tool. + SEE ALSO -------- linkgit:git-diff[1]:: diff --git a/git-difftool-helper.sh b/git-difftool-helper.sh index b91002b..02bb135 100755 --- a/git-difftool-helper.sh +++ b/git-difftool-helper.sh @@ -7,9 +7,15 @@ # # Copyright (c) 2009 David Aguilar -# Set GIT_DIFFTOOL_NO_PROMPT to bypass the per-file prompt. +# difftool.prompt controls the default prompt/no-prompt behavior +# and is overridden with $GIT_DIFFTOOL*_PROMPT. should_prompt () { - test -z "$GIT_DIFFTOOL_NO_PROMPT" + prompt=$(git config --bool difftool.prompt || echo true) + if test "$prompt" = true; then + test -z "$GIT_DIFFTOOL_NO_PROMPT" + else + test -n "$GIT_DIFFTOOL_PROMPT" + fi } # This function prepares temporary files and launches the appropriate diff --git a/git-difftool.perl b/git-difftool.perl index 8c160e5..985dfe0 100755 --- a/git-difftool.perl +++ b/git-difftool.perl @@ -2,9 +2,12 @@ # Copyright (c) 2009 David Aguilar # # This is a wrapper around the GIT_EXTERNAL_DIFF-compatible -# git-difftool-helper script. This script exports -# GIT_EXTERNAL_DIFF and GIT_PAGER for use by git, and -# GIT_DIFFTOOL_NO_PROMPT and GIT_DIFF_TOOL for use by git-difftool-helper. +# git-difftool-helper script. +# +# This script exports GIT_EXTERNAL_DIFF and GIT_PAGER for use by git. +# GIT_DIFFTOOL_NO_PROMPT, GIT_DIFFTOOL_PROMPT, and GIT_DIFF_TOOL +# are exported for use by git-difftool-helper. +# # Any arguments that are unknown to this script are forwarded to 'git diff'. use strict; @@ -62,6 +65,12 @@ sub generate_command } if ($arg eq '-y' || $arg eq '--no-prompt') { $ENV{GIT_DIFFTOOL_NO_PROMPT} = 'true'; + delete $ENV{GIT_DIFFTOOL_PROMPT}; + next; + } + if ($arg eq '--prompt') { + $ENV{GIT_DIFFTOOL_PROMPT} = 'true'; + delete $ENV{GIT_DIFFTOOL_NO_PROMPT}; next; } if ($arg eq '-h' || $arg eq '--help') { diff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh index ceef84b..33d07e6 100755 --- a/t/t7800-difftool.sh +++ b/t/t7800-difftool.sh @@ -15,6 +15,7 @@ remove_config_vars() # Unset all config variables used by git-difftool git config --unset diff.tool git config --unset difftool.test-tool.cmd + git config --unset difftool.prompt git config --unset merge.tool git config --unset mergetool.test-tool.cmd return 0 @@ -26,11 +27,18 @@ restore_test_defaults() remove_config_vars unset GIT_DIFF_TOOL unset GIT_MERGE_TOOL + unset GIT_DIFFTOOL_PROMPT unset GIT_DIFFTOOL_NO_PROMPT git config diff.tool test-tool && git config difftool.test-tool.cmd 'cat $LOCAL' } +prompt_given() +{ + prompt="$1" + test "$prompt" = "Hit return to launch 'test-tool': branch" +} + # Create a file on master and change it on branch test_expect_success 'setup' ' echo master >file && @@ -116,6 +124,62 @@ test_expect_success 'GIT_DIFFTOOL_NO_PROMPT variable' ' restore_test_defaults ' +# git-difftool supports the difftool.prompt variable. +# Test that GIT_DIFFTOOL_PROMPT can override difftool.prompt = false +test_expect_success 'GIT_DIFFTOOL_PROMPT variable' ' + git config difftool.prompt false && + GIT_DIFFTOOL_PROMPT=true && + export GIT_DIFFTOOL_PROMPT && + + prompt=$(echo | git difftool --prompt branch | tail -1) && + prompt_given "$prompt" && + + restore_test_defaults +' + +# Test that we don't have to pass --no-prompt when difftool.prompt is false +test_expect_success 'difftool.prompt config variable is false' ' + git config difftool.prompt false && + + diff=$(git difftool branch) && + test "$diff" = "branch" && + + restore_test_defaults +' + +# Test that the -y flag can override difftool.prompt = true +test_expect_success 'difftool.prompt can overridden with -y' ' + git config difftool.prompt true && + + diff=$(git difftool -y branch) && + test "$diff" = "branch" && + + restore_test_defaults +' + +# Test that the --prompt flag can override difftool.prompt = false +test_expect_success 'difftool.prompt can overridden with --prompt' ' + git config difftool.prompt false && + + prompt=$(echo | git difftool --prompt branch | tail -1) && + prompt_given "$prompt" && + + restore_test_defaults +' + +# Test that the last flag passed on the command-line wins +test_expect_success 'difftool last flag wins' ' + diff=$(git difftool --prompt --no-prompt branch) && + test "$diff" = "branch" && + + restore_test_defaults && + + prompt=$(echo | git difftool --no-prompt --prompt branch | tail -1) && + prompt_given "$prompt" && + + restore_test_defaults +' + # git-difftool falls back to git-mergetool config variables # so test that behavior here test_expect_success 'difftool + mergetool config variables' ' -- 1.6.1.3 ^ permalink raw reply related [flat|nested] 26+ messages in thread
* [PATCH 02/10] mergetool: use $( ... ) instead of `backticks` 2009-04-01 12:55 ` [PATCH 01/10] difftool: add support for a difftool.prompt config variable David Aguilar @ 2009-04-01 12:55 ` David Aguilar 2009-04-01 12:55 ` [PATCH 03/10] Add a mergetool-lib scriptlet for holding common merge tool functions David Aguilar 0 siblings, 1 reply; 26+ messages in thread From: David Aguilar @ 2009-04-01 12:55 UTC (permalink / raw) To: gitster, charles; +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 87fa88a..7c04031 100755 --- a/git-mergetool.sh +++ b/git-mergetool.sh @@ -137,7 +137,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" @@ -156,9 +156,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" @@ -308,7 +308,7 @@ do -t|--tool*) case "$#,$1" in *,*=*) - merge_tool=`expr "z$1" : 'z-[^=]*=\(.*\)'` + merge_tool=$(expr "z$1" : 'z-[^=]*=\(.*\)') ;; 1,*) usage ;; @@ -356,7 +356,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) @@ -387,7 +387,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..." @@ -447,7 +447,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] 26+ messages in thread
* [PATCH 03/10] Add a mergetool-lib scriptlet for holding common merge tool functions 2009-04-01 12:55 ` [PATCH 02/10] mergetool: use $( ... ) instead of `backticks` David Aguilar @ 2009-04-01 12:55 ` David Aguilar 2009-04-01 12:55 ` [PATCH 04/10] mergetool: use get_mergetool_path from git-mergetool-lib David Aguilar 2009-04-01 22:39 ` [PATCH 03/10] Add a mergetool-lib scriptlet for holding common merge tool functions Markus Heidelberg 0 siblings, 2 replies; 26+ messages in thread From: David Aguilar @ 2009-04-01 12:55 UTC (permalink / raw) To: gitster, charles; +Cc: git, David Aguilar git-mergetool-lib provides common merge tool functions. Signed-off-by: David Aguilar <davvid@gmail.com> --- .gitignore | 1 + Documentation/git-mergetool-lib.txt | 42 +++++++++++++++++++++++++++++++++++ Makefile | 1 + command-list.txt | 1 + git-mergetool-lib.sh | 41 ++++++++++++++++++++++++++++++++++ 5 files changed, 86 insertions(+), 0 deletions(-) create mode 100644 Documentation/git-mergetool-lib.txt create mode 100644 git-mergetool-lib.sh diff --git a/.gitignore b/.gitignore index 966c886..75c154a 100644 --- a/.gitignore +++ b/.gitignore @@ -80,6 +80,7 @@ git-merge-recursive git-merge-resolve git-merge-subtree git-mergetool +git-mergetool-lib git-mktag git-mktree git-name-rev diff --git a/Documentation/git-mergetool-lib.txt b/Documentation/git-mergetool-lib.txt new file mode 100644 index 0000000..a8d62f5 --- /dev/null +++ b/Documentation/git-mergetool-lib.txt @@ -0,0 +1,42 @@ +git-mergetool-lib(1) +==================== + +NAME +---- +git-mergetool-lib - Common git merge tool shell scriptlets + +SYNOPSIS +-------- +'. "$(git --exec-path)/git-mergetool-lib"' + +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-mergetool-lib' scriptlet is designed to be sourced (using +`.`) by other shell scripts to set up functions for working +with git merge 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 +--------- +get_merge_tool_path:: + returns the `merge_tool_path` for a `merge_tool`. + +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..086f9e7 100644 --- a/Makefile +++ b/Makefile @@ -284,6 +284,7 @@ SCRIPT_SH += git-merge-octopus.sh SCRIPT_SH += git-merge-one-file.sh SCRIPT_SH += git-merge-resolve.sh SCRIPT_SH += git-mergetool.sh +SCRIPT_SH += git-mergetool-lib.sh SCRIPT_SH += git-parse-remote.sh SCRIPT_SH += git-pull.sh SCRIPT_SH += git-quiltimport.sh diff --git a/command-list.txt b/command-list.txt index fb03a2e..922c815 100644 --- a/command-list.txt +++ b/command-list.txt @@ -69,6 +69,7 @@ git-merge-file plumbingmanipulators git-merge-index plumbingmanipulators git-merge-one-file purehelpers git-mergetool ancillarymanipulators +git-mergetool-lib purehelpers git-merge-tree ancillaryinterrogators git-mktag plumbingmanipulators git-mktree plumbingmanipulators diff --git a/git-mergetool-lib.sh b/git-mergetool-lib.sh new file mode 100644 index 0000000..c307a5d --- /dev/null +++ b/git-mergetool-lib.sh @@ -0,0 +1,41 @@ +# git-mergetool-lib is a library for common merge tool functions +diff_mode() { + test $TOOL_MODE = "diff" +} + +get_merge_tool_path () { + if test -z "$2"; then + case "$1" in + emerge) + path=emacs + ;; + *) + path="$1" + ;; + esac + fi + echo "$path" +} + +valid_tool () { + case "$1" in + kdiff3 | kompare | tkdiff | xxdiff | meld | opendiff | emerge | vimdiff | gvimdiff | ecmerge) + if test "$1" = "kompare" && ! diff_mode; then + return 1 + fi + ;; # happy + *) + if ! test -n "$(get_custom_cmd "$1")"; then + return 1 + fi ;; + esac +} + +get_custom_cmd () { + diff_mode && + custom_cmd="$(git config difftool.$1.cmd)" + test -z "$custom_cmd" && + custom_cmd="$(git config mergetool.$1.cmd)" + test -n "$custom_cmd" && + echo "$custom_cmd" +} -- 1.6.1.3 ^ permalink raw reply related [flat|nested] 26+ messages in thread
* [PATCH 04/10] mergetool: use get_mergetool_path from git-mergetool-lib 2009-04-01 12:55 ` [PATCH 03/10] Add a mergetool-lib scriptlet for holding common merge tool functions David Aguilar @ 2009-04-01 12:55 ` David Aguilar 2009-04-01 12:55 ` [PATCH 05/10] difftool: " David Aguilar 2009-04-01 22:39 ` [PATCH 03/10] Add a mergetool-lib scriptlet for holding common merge tool functions Markus Heidelberg 1 sibling, 1 reply; 26+ messages in thread From: David Aguilar @ 2009-04-01 12:55 UTC (permalink / raw) To: gitster, charles; +Cc: git, David Aguilar This refactors git-mergetool to use get_mergetool_path(). Signed-off-by: David Aguilar <davvid@gmail.com> --- git-mergetool.sh | 20 +++----------------- 1 files changed, 3 insertions(+), 17 deletions(-) diff --git a/git-mergetool.sh b/git-mergetool.sh index 7c04031..732a5b7 100755 --- a/git-mergetool.sh +++ b/git-mergetool.sh @@ -12,6 +12,7 @@ USAGE='[--tool=tool] [-y|--no-prompt|--prompt] [file to merge] ...' SUBDIRECTORY_OK=Yes OPTIONS_SPEC= . git-sh-setup +. git-mergetool-lib require_work_tree # Returns true if the mode reflects a symlink @@ -355,20 +356,6 @@ valid_tool() { 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) ? " @@ -412,7 +399,7 @@ if test -z "$merge_tool" ; then fi echo "merge tool candidates: $merge_tool_candidates" for i in $merge_tool_candidates; do - init_merge_tool_path $i + merge_tool_path="$(get_merge_tool_path "$i" "$merge_tool_path")" if type "$merge_tool_path" > /dev/null 2>&1; then merge_tool=$i break @@ -428,8 +415,7 @@ else exit 1 fi - init_merge_tool_path "$merge_tool" - + merge_tool_path="$(get_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)" -- 1.6.1.3 ^ permalink raw reply related [flat|nested] 26+ messages in thread
* [PATCH 05/10] difftool: use get_mergetool_path from git-mergetool-lib 2009-04-01 12:55 ` [PATCH 04/10] mergetool: use get_mergetool_path from git-mergetool-lib David Aguilar @ 2009-04-01 12:55 ` David Aguilar 2009-04-01 12:55 ` [PATCH 06/10] mergetool: use valid_tool " David Aguilar 0 siblings, 1 reply; 26+ messages in thread From: David Aguilar @ 2009-04-01 12:55 UTC (permalink / raw) To: gitster, charles; +Cc: git, David Aguilar This refactors git-difftool-helper to use get_mergetool_path(). Signed-off-by: David Aguilar <davvid@gmail.com> --- git-difftool-helper.sh | 33 ++++++++++++--------------------- 1 files changed, 12 insertions(+), 21 deletions(-) diff --git a/git-difftool-helper.sh b/git-difftool-helper.sh index 02bb135..d1bea18 100755 --- a/git-difftool-helper.sh +++ b/git-difftool-helper.sh @@ -7,6 +7,9 @@ # # Copyright (c) 2009 David Aguilar +# Load common functions from git-mergetool-lib +. git-mergetool-lib + # difftool.prompt controls the default prompt/no-prompt behavior # and is overridden with $GIT_DIFFTOOL*_PROMPT. should_prompt () { @@ -125,25 +128,6 @@ valid_tool() { 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" @@ -187,7 +171,7 @@ if test -z "$merge_tool"; then # Loop over each candidate and stop when a valid merge tool is found. for i in $merge_tool_candidates do - init_merge_tool_path $i + merge_tool_path="$(get_merge_tool_path "$i")" if type "$merge_tool_path" > /dev/null 2>&1; then merge_tool=$i break @@ -206,7 +190,14 @@ else exit 1 fi - init_merge_tool_path "$merge_tool" + # Sets up the merge_tool_path variable. + # This handles the difftool.<tool>.path configuration variable + # and falls back to mergetool defaults. + merge_tool_path=$(git config difftool."$1".path) + test -z "$merge_tool_path" && + merge_tool_path=$(git config mergetool."$1".path) + merge_tool_path="$(get_merge_tool_path "$merge_tool" \ + "$merge_tool_path")" 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'" -- 1.6.1.3 ^ permalink raw reply related [flat|nested] 26+ messages in thread
* [PATCH 06/10] mergetool: use valid_tool from git-mergetool-lib 2009-04-01 12:55 ` [PATCH 05/10] difftool: " David Aguilar @ 2009-04-01 12:55 ` David Aguilar 2009-04-01 12:55 ` [PATCH 07/10] difftool: " David Aguilar 0 siblings, 1 reply; 26+ messages in thread From: David Aguilar @ 2009-04-01 12:55 UTC (permalink / raw) To: gitster, charles; +Cc: git, David Aguilar This refactors git-mergetool to use valid_tool from git-mergetool-lib. Signed-off-by: David Aguilar <davvid@gmail.com> --- git-mergetool.sh | 21 +++------------------ 1 files changed, 3 insertions(+), 18 deletions(-) diff --git a/git-mergetool.sh b/git-mergetool.sh index 732a5b7..957993c 100755 --- a/git-mergetool.sh +++ b/git-mergetool.sh @@ -11,6 +11,7 @@ USAGE='[--tool=tool] [-y|--no-prompt|--prompt] [file to merge] ...' SUBDIRECTORY_OK=Yes OPTIONS_SPEC= +TOOL_MODE=merge . git-sh-setup . git-mergetool-lib require_work_tree @@ -338,24 +339,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 -} - prompt_after_failed_merge() { while true; do printf "Continue merging other unresolved paths (y/n) ? " @@ -409,12 +392,14 @@ if test -z "$merge_tool" ; then echo "No known merge resolution program available." exit 1 fi + merge_tool_cmd="$(git config mergetool."$merge_tool".cmd)" else if ! valid_tool "$merge_tool"; then echo >&2 "Unknown merge_tool $merge_tool" exit 1 fi + merge_tool_cmd="$(git config mergetool.$merge_tool.cmd)" merge_tool_path="$(get_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)" -- 1.6.1.3 ^ permalink raw reply related [flat|nested] 26+ messages in thread
* [PATCH 07/10] difftool: use valid_tool from git-mergetool-lib 2009-04-01 12:55 ` [PATCH 06/10] mergetool: use valid_tool " David Aguilar @ 2009-04-01 12:55 ` David Aguilar 2009-04-01 12:55 ` [PATCH 08/10] mergetool-lib: introduce run_mergetool David Aguilar 0 siblings, 1 reply; 26+ messages in thread From: David Aguilar @ 2009-04-01 12:55 UTC (permalink / raw) To: gitster, charles; +Cc: git, David Aguilar This refactors difftool to use valid_tool fro git-mergetool-lib Signed-off-by: David Aguilar <davvid@gmail.com> --- git-difftool-helper.sh | 29 +++-------------------------- 1 files changed, 3 insertions(+), 26 deletions(-) diff --git a/git-difftool-helper.sh b/git-difftool-helper.sh index d1bea18..6cc5ab5 100755 --- a/git-difftool-helper.sh +++ b/git-difftool-helper.sh @@ -8,6 +8,7 @@ # Copyright (c) 2009 David Aguilar # Load common functions from git-mergetool-lib +TOOL_MODE=diff . git-mergetool-lib # difftool.prompt controls the default prompt/no-prompt behavior @@ -105,29 +106,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 -} - # 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" @@ -183,6 +161,7 @@ if test -z "$merge_tool"; then exit 1 fi + merge_tool_cmd=$(get_custom_cmd "$merge_tool") else # A merge tool has been set, so verify that it's valid. if ! valid_tool "$merge_tool"; then @@ -190,9 +169,7 @@ else exit 1 fi - # Sets up the merge_tool_path variable. - # This handles the difftool.<tool>.path configuration variable - # and falls back to mergetool defaults. + merge_tool_cmd=$(get_custom_cmd "$merge_tool") merge_tool_path=$(git config difftool."$1".path) test -z "$merge_tool_path" && merge_tool_path=$(git config mergetool."$1".path) -- 1.6.1.3 ^ permalink raw reply related [flat|nested] 26+ messages in thread
* [PATCH 08/10] mergetool-lib: introduce run_mergetool 2009-04-01 12:55 ` [PATCH 07/10] difftool: " David Aguilar @ 2009-04-01 12:55 ` David Aguilar 2009-04-01 12:55 ` [PATCH 09/10] difftool: use run_mergetool from git-mergetool-lib David Aguilar 2009-04-01 22:47 ` [PATCH 08/10] mergetool-lib: introduce run_mergetool Markus Heidelberg 0 siblings, 2 replies; 26+ messages in thread From: David Aguilar @ 2009-04-01 12:55 UTC (permalink / raw) To: gitster, charles; +Cc: git, David Aguilar run_mergetool contains the common functionality for launching mergetools. There's some context-specific behavior but overall it's better to have all of this stuff in one place versus multiple places. Signed-off-by: David Aguilar <davvid@gmail.com> --- Documentation/git-mergetool-lib.txt | 4 + git-mergetool-lib.sh | 141 +++++++++++++++++++++++++++++++++++ 2 files changed, 145 insertions(+), 0 deletions(-) diff --git a/Documentation/git-mergetool-lib.txt b/Documentation/git-mergetool-lib.txt index a8d62f5..3060673 100644 --- a/Documentation/git-mergetool-lib.txt +++ b/Documentation/git-mergetool-lib.txt @@ -29,6 +29,10 @@ FUNCTIONS get_merge_tool_path:: returns the `merge_tool_path` for a `merge_tool`. +run_mergetool:: + launches a merge tool given the tool name and a true/false + flag to indicate whether a merge base is present + Author ------ Written by David Aguilar <davvid@gmail.com> diff --git a/git-mergetool-lib.sh b/git-mergetool-lib.sh index c307a5d..9c26b5f 100644 --- a/git-mergetool-lib.sh +++ b/git-mergetool-lib.sh @@ -3,7 +3,12 @@ diff_mode() { test $TOOL_MODE = "diff" } +merge_mode() { + test $TOOL_MODE = "merge" +} + get_merge_tool_path () { + path="$1" if test -z "$2"; then case "$1" in emerge) @@ -17,6 +22,11 @@ get_merge_tool_path () { echo "$path" } +# Overridden in git-mergetool +check_unchanged () { + true +} + valid_tool () { case "$1" in kdiff3 | kompare | tkdiff | xxdiff | meld | opendiff | emerge | vimdiff | gvimdiff | ecmerge) @@ -39,3 +49,134 @@ get_custom_cmd () { test -n "$custom_cmd" && echo "$custom_cmd" } + +run_mergetool () { + base_present="$2" + if diff_mode; then + base_present="false" + fi + if test -z "$base_present"; then + base_present="true" + fi + + case "$1" 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=$? + ;; + kompare) + "$merge_tool_path" "$LOCAL" "$REMOTE" + status=$? + ;; + tkdiff) + if $base_present; then + "$merge_tool_path" -a "$BASE" -o "$MERGED" "$LOCAL" "$REMOTE" + else + if diff_mode; then + "$merge_tool_path" "$LOCAL" "$REMOTE" + else + "$merge_tool_path" -o "$MERGED" "$LOCAL" "$REMOTE" + fi + fi + status=$? + ;; + meld) + merge_mode && touch "$BACKUP" + if merge_mode; then + "$merge_tool_path" "$LOCAL" "$MERGED" "$REMOTE" + else + "$merge_tool_path" "$LOCAL" "$REMOTE" + fi + check_unchanged + ;; + vimdiff) + if merge_mode; then + touch "$BACKUP" + "$merge_tool_path" -c "wincmd l" "$LOCAL" "$MERGED" "$REMOTE" + check_unchanged + else + "$merge_tool_path" -c "wincmd l" "$LOCAL" "$REMOTE" + fi + ;; + gvimdiff) + if merge_mode; then + touch "$BACKUP" + "$merge_tool_path" -c "wincmd l" -f "$LOCAL" "$MERGED" "$REMOTE" + check_unchanged + else + "$merge_tool_path" -c "wincmd l" -f "$LOCAL" "$REMOTE" + fi + ;; + xxdiff) + merge_mode && 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_mode && extra=--show-merged-pane + "$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 + ;; + opendiff) + merge_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 + check_unchanged + ;; + ecmerge) + merge_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 + 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 merge_mode && + 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 +} -- 1.6.1.3 ^ permalink raw reply related [flat|nested] 26+ messages in thread
* [PATCH 09/10] difftool: use run_mergetool from git-mergetool-lib 2009-04-01 12:55 ` [PATCH 08/10] mergetool-lib: introduce run_mergetool David Aguilar @ 2009-04-01 12:55 ` David Aguilar 2009-04-01 12:55 ` [PATCH 10/10] mergetool: " David Aguilar 2009-04-01 22:47 ` [PATCH 08/10] mergetool-lib: introduce run_mergetool Markus Heidelberg 1 sibling, 1 reply; 26+ messages in thread From: David Aguilar @ 2009-04-01 12:55 UTC (permalink / raw) To: gitster, charles; +Cc: git, David Aguilar This refactors git-mergetool-helper to use run_mergetool. Signed-off-by: David Aguilar <davvid@gmail.com> --- git-difftool-helper.sh | 62 +----------------------------------------------- 1 files changed, 1 insertions(+), 61 deletions(-) diff --git a/git-difftool-helper.sh b/git-difftool-helper.sh index 6cc5ab5..2dfc870 100755 --- a/git-difftool-helper.sh +++ b/git-difftool-helper.sh @@ -43,67 +43,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_mergetool "$merge_tool"; } # Allow GIT_DIFF_TOOL and GIT_MERGE_TOOL to provide default values -- 1.6.1.3 ^ permalink raw reply related [flat|nested] 26+ messages in thread
* [PATCH 10/10] mergetool: use run_mergetool from git-mergetool-lib 2009-04-01 12:55 ` [PATCH 09/10] difftool: use run_mergetool from git-mergetool-lib David Aguilar @ 2009-04-01 12:55 ` David Aguilar 2009-04-01 22:54 ` Markus Heidelberg 0 siblings, 1 reply; 26+ messages in thread From: David Aguilar @ 2009-04-01 12:55 UTC (permalink / raw) To: gitster, charles; +Cc: git, David Aguilar This refactors git-mergetool to use run_mergetool. Signed-off-by: David Aguilar <davvid@gmail.com> --- git-mergetool.sh | 96 +++-------------------------------------------------- 1 files changed, 6 insertions(+), 90 deletions(-) diff --git a/git-mergetool.sh b/git-mergetool.sh index 957993c..2c6b325 100755 --- a/git-mergetool.sh +++ b/git-mergetool.sh @@ -190,96 +190,12 @@ 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 + present=false + base_present && + present=true + + run_mergetool "$merge_tool" "$present" + 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] 26+ messages in thread
* Re: [PATCH 10/10] mergetool: use run_mergetool from git-mergetool-lib 2009-04-01 12:55 ` [PATCH 10/10] mergetool: " David Aguilar @ 2009-04-01 22:54 ` Markus Heidelberg 2009-04-02 20:02 ` Charles Bailey 0 siblings, 1 reply; 26+ messages in thread From: Markus Heidelberg @ 2009-04-01 22:54 UTC (permalink / raw) To: David Aguilar; +Cc: gitster, charles, git David Aguilar, 01.04.2009: > This refactors git-mergetool to use run_mergetool. > > Signed-off-by: David Aguilar <davvid@gmail.com> > --- > git-mergetool.sh | 96 +++-------------------------------------------------- > 1 files changed, 6 insertions(+), 90 deletions(-) > > diff --git a/git-mergetool.sh b/git-mergetool.sh > index 957993c..2c6b325 100755 > --- a/git-mergetool.sh > +++ b/git-mergetool.sh > @@ -190,96 +190,12 @@ 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 > + present=false > + base_present && > + present=true > + > + run_mergetool "$merge_tool" "$present" > + status=$? This last line has to be deleted, because the variable 'status' set in run_mergetool would be overwritten then. In this case the merge will succeed even if it didn't and the file will be staged. > if test "$status" -ne 0; then > echo "merge of $MERGED failed" 1>&2 > mv -- "$BACKUP" "$MERGED" ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 10/10] mergetool: use run_mergetool from git-mergetool-lib 2009-04-01 22:54 ` Markus Heidelberg @ 2009-04-02 20:02 ` Charles Bailey 2009-04-02 20:13 ` Markus Heidelberg 0 siblings, 1 reply; 26+ messages in thread From: Charles Bailey @ 2009-04-02 20:02 UTC (permalink / raw) To: Markus Heidelberg; +Cc: David Aguilar, gitster, git On Thu, Apr 02, 2009 at 12:54:22AM +0200, Markus Heidelberg wrote: > David Aguilar, 01.04.2009: > > + present=false > > + base_present && > > + present=true > > + > > + run_mergetool "$merge_tool" "$present" > > + status=$? > > This last line has to be deleted, because the variable 'status' set in > run_mergetool would be overwritten then. In this case the merge will > succeed even if it didn't and the file will be staged. I think that it would be better if $status stayed as local as possible. If run_mergetool returned the value of $status as its exit code then the function will properly return whether the merge succeeded or not. This way run_mergetool's clients can just use its exit code, without having to know about a magic $status global variable. -- Charles Bailey http://ccgi.hashpling.plus.com/blog/ ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 10/10] mergetool: use run_mergetool from git-mergetool-lib 2009-04-02 20:02 ` Charles Bailey @ 2009-04-02 20:13 ` Markus Heidelberg 2009-04-02 20:16 ` Markus Heidelberg 2009-04-03 1:54 ` David Aguilar 0 siblings, 2 replies; 26+ messages in thread From: Markus Heidelberg @ 2009-04-02 20:13 UTC (permalink / raw) To: Charles Bailey; +Cc: David Aguilar, gitster, git Charles Bailey, 02.04.2009: > On Thu, Apr 02, 2009 at 12:54:22AM +0200, Markus Heidelberg wrote: > > David Aguilar, 01.04.2009: > > > + present=false > > > + base_present && > > > + present=true > > > + > > > + run_mergetool "$merge_tool" "$present" > > > + status=$? > > > > This last line has to be deleted, because the variable 'status' set in > > run_mergetool would be overwritten then. In this case the merge will > > succeed even if it didn't and the file will be staged. > > I think that it would be better if $status stayed as local as > possible. If run_mergetool returned the value of $status as its exit > code then the function will properly return whether the merge > succeeded or not. > > This way run_mergetool's clients can just use its exit code, without > having to know about a magic $status global variable. Sure. Next time I have to think about the right solution before spreading workarounds. And shouldn't also check_unchanged() move from git-mergetool.sh to git-mergetool--lib.sh, since it is only used there? Huh, it seems as if some editors for difftool cannot work correctly currently, because this function is used, but defined in git-mergetool.sh. Markus ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 10/10] mergetool: use run_mergetool from git-mergetool-lib 2009-04-02 20:13 ` Markus Heidelberg @ 2009-04-02 20:16 ` Markus Heidelberg 2009-04-03 1:54 ` David Aguilar 1 sibling, 0 replies; 26+ messages in thread From: Markus Heidelberg @ 2009-04-02 20:16 UTC (permalink / raw) To: Charles Bailey; +Cc: David Aguilar, gitster, git Markus Heidelberg, 02.04.2009: > Huh, it seems as if some editors for difftool cannot work correctly > currently, because this function is used, but defined in > git-mergetool.sh. Yes, it works... # Overridden in git-mergetool check_unchanged () { true } But that's not really nice, is it? Maybe we'd be better with a if merge_mode; then inside this function and only define it in git-mergetool--lib.sh. Markus ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 10/10] mergetool: use run_mergetool from git-mergetool-lib 2009-04-02 20:13 ` Markus Heidelberg 2009-04-02 20:16 ` Markus Heidelberg @ 2009-04-03 1:54 ` David Aguilar 1 sibling, 0 replies; 26+ messages in thread From: David Aguilar @ 2009-04-03 1:54 UTC (permalink / raw) To: Markus Heidelberg; +Cc: Charles Bailey, gitster, git On 0, Markus Heidelberg <markus.heidelberg@web.de> wrote: > Charles Bailey, 02.04.2009: > > > > I think that it would be better if $status stayed as local as > > possible. If run_mergetool returned the value of $status as its exit > > code then the function will properly return whether the merge > > succeeded or not. > > > > This way run_mergetool's clients can just use its exit code, without > > having to know about a magic $status global variable. > > And shouldn't also check_unchanged() move from git-mergetool.sh to > git-mergetool--lib.sh, since it is only used there? > Huh, it seems as if some editors for difftool cannot work correctly > currently, because this function is used, but defined in > git-mergetool.sh. Sounds good. A patch is on the way: - moves check_unchanged into mergetool--lib - returns $status in run_mergetool - updates mergetool to not rely on the global $status -- David ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 08/10] mergetool-lib: introduce run_mergetool 2009-04-01 12:55 ` [PATCH 08/10] mergetool-lib: introduce run_mergetool David Aguilar 2009-04-01 12:55 ` [PATCH 09/10] difftool: use run_mergetool from git-mergetool-lib David Aguilar @ 2009-04-01 22:47 ` Markus Heidelberg 1 sibling, 0 replies; 26+ messages in thread From: Markus Heidelberg @ 2009-04-01 22:47 UTC (permalink / raw) To: David Aguilar; +Cc: gitster, charles, git David Aguilar, 01.04.2009: > diff --git a/git-mergetool-lib.sh b/git-mergetool-lib.sh > +run_mergetool () { > + base_present="$2" > + if diff_mode; then > + base_present="false" > + fi > + if test -z "$base_present"; then > + base_present="true" > + fi > + > + case "$1" 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=$? > + ;; > + kompare) > + "$merge_tool_path" "$LOCAL" "$REMOTE" > + status=$? > + ;; > + tkdiff) > + if $base_present; then > + "$merge_tool_path" -a "$BASE" -o "$MERGED" "$LOCAL" "$REMOTE" > + else > + if diff_mode; then Query merge_mode instead of diff_mode as in all the other places. > + "$merge_tool_path" "$LOCAL" "$REMOTE" > + else > + "$merge_tool_path" -o "$MERGED" "$LOCAL" "$REMOTE" > + fi > + fi > + status=$? > + ;; > + meld) > + merge_mode && touch "$BACKUP" > + if merge_mode; then > + "$merge_tool_path" "$LOCAL" "$MERGED" "$REMOTE" Put the touch command into the if then block as in the other places. > + else > + "$merge_tool_path" "$LOCAL" "$REMOTE" > + fi > + check_unchanged > + ;; > + vimdiff) > + if merge_mode; then > + touch "$BACKUP" > + "$merge_tool_path" -c "wincmd l" "$LOCAL" "$MERGED" "$REMOTE" > + check_unchanged > + else > + "$merge_tool_path" -c "wincmd l" "$LOCAL" "$REMOTE" > + fi > + ;; > + gvimdiff) > + if merge_mode; then > + touch "$BACKUP" > + "$merge_tool_path" -c "wincmd l" -f "$LOCAL" "$MERGED" "$REMOTE" > + check_unchanged > + else > + "$merge_tool_path" -c "wincmd l" -f "$LOCAL" "$REMOTE" > + fi > + ;; > + xxdiff) > + merge_mode && 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_mode && extra=--show-merged-pane > + "$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 > + ;; > + opendiff) > + merge_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 > + check_unchanged > + ;; > + ecmerge) > + merge_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 > + 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 merge_mode && > + 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 > +} ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 03/10] Add a mergetool-lib scriptlet for holding common merge tool functions 2009-04-01 12:55 ` [PATCH 03/10] Add a mergetool-lib scriptlet for holding common merge tool functions David Aguilar 2009-04-01 12:55 ` [PATCH 04/10] mergetool: use get_mergetool_path from git-mergetool-lib David Aguilar @ 2009-04-01 22:39 ` Markus Heidelberg 2009-04-02 3:58 ` David Aguilar 1 sibling, 1 reply; 26+ messages in thread From: Markus Heidelberg @ 2009-04-01 22:39 UTC (permalink / raw) To: David Aguilar; +Cc: gitster, charles, git David Aguilar, 01.04.2009: > > diff --git a/git-mergetool-lib.sh b/git-mergetool-lib.sh > +valid_tool () { > + case "$1" in > + kdiff3 | kompare | tkdiff | xxdiff | meld | opendiff | emerge | vimdiff | gvimdiff | ecmerge) > + if test "$1" = "kompare" && ! diff_mode; then > + return 1 > + fi > + ;; # happy > + *) > + if ! test -n "$(get_custom_cmd "$1")"; then Better this? if test -z "$(get_custom_cmd "$1")"; then > + return 1 > + fi ;; For consistency: fi ;; ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 03/10] Add a mergetool-lib scriptlet for holding common merge tool functions 2009-04-01 22:39 ` [PATCH 03/10] Add a mergetool-lib scriptlet for holding common merge tool functions Markus Heidelberg @ 2009-04-02 3:58 ` David Aguilar 0 siblings, 0 replies; 26+ messages in thread From: David Aguilar @ 2009-04-02 3:58 UTC (permalink / raw) To: Markus Heidelberg; +Cc: gitster, charles, git On 0, Markus Heidelberg <markus.heidelberg@web.de> wrote: > David Aguilar, 01.04.2009: > > > > diff --git a/git-mergetool-lib.sh b/git-mergetool-lib.sh > > +valid_tool () { > > + case "$1" in > > + kdiff3 | kompare | tkdiff | xxdiff | meld | opendiff | emerge | vimdiff | gvimdiff | ecmerge) > > + if test "$1" = "kompare" && ! diff_mode; then > > + return 1 > > + fi > > + ;; # happy > > + *) > > + if ! test -n "$(get_custom_cmd "$1")"; then > > Better this? > if test -z "$(get_custom_cmd "$1")"; then > > > + return 1 > > + fi ;; > > For consistency: > fi > ;; > I'll reroll tonight and include you on the CC when I resent the patches in question. Thanks Markus. -- David ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: git-{diff,merge} refactor round 2 2009-04-01 12:55 git-{diff,merge} refactor round 2 David Aguilar 2009-04-01 12:55 ` [PATCH 01/10] difftool: add support for a difftool.prompt config variable David Aguilar @ 2009-04-02 19:59 ` Charles Bailey 2009-04-05 2:58 ` Markus Heidelberg 2 siblings, 0 replies; 26+ messages in thread From: Charles Bailey @ 2009-04-02 19:59 UTC (permalink / raw) To: David Aguilar; +Cc: gitster, git On Wed, Apr 01, 2009 at 05:55:04AM -0700, David Aguilar wrote: > Here's the 2nd round of refactoring. Again, I reply in summary form... [02/10] - I ack this. [03-07 / 10] - fine. I'm not sure, but should they be one commit? They're really about consolidating blocks from one place to another and the change might be more 'obvious' as a single commit. [08/10] Now that we've isolated the run_mergetool, can we get it to return the value of '$status' (either the result of the tool or of check_unchanged) as its exit code so that we know whether the merge succeeded? [10/10] Then this chunk: + run_mergetool "$merge_tool" "$present" + status=$? if test "$status" -ne 0; then can validly become: if ! run_mergtool "$merge_tool" "$present"; then which seems clearer to me. -- Charles Bailey http://ccgi.hashpling.plus.com/blog/ ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: git-{diff,merge} refactor round 2 2009-04-01 12:55 git-{diff,merge} refactor round 2 David Aguilar 2009-04-01 12:55 ` [PATCH 01/10] difftool: add support for a difftool.prompt config variable David Aguilar 2009-04-02 19:59 ` git-{diff,merge} refactor round 2 Charles Bailey @ 2009-04-05 2:58 ` Markus Heidelberg 2009-04-05 3:34 ` David Aguilar 2 siblings, 1 reply; 26+ messages in thread From: Markus Heidelberg @ 2009-04-05 2:58 UTC (permalink / raw) To: David Aguilar; +Cc: gitster, charles, git David Aguilar, 01.04.2009: > Here's the 2nd round of refactoring. I just noticed that mergetool.<mergetool>.path doesn't work anymore. git grep mergetool.*path only hits one line in git-difftool--helper.sh Neither does it seem to work with difftool, but I'm gonna go to bed now. Markus ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: git-{diff,merge} refactor round 2 2009-04-05 2:58 ` Markus Heidelberg @ 2009-04-05 3:34 ` David Aguilar 2009-04-05 9:45 ` Junio C Hamano 0 siblings, 1 reply; 26+ messages in thread From: David Aguilar @ 2009-04-05 3:34 UTC (permalink / raw) To: Markus Heidelberg; +Cc: gitster, charles, git On 0, Markus Heidelberg <markus.heidelberg@web.de> wrote: > David Aguilar, 01.04.2009: > > Here's the 2nd round of refactoring. > > I just noticed that mergetool.<mergetool>.path doesn't work anymore. > git grep mergetool.*path only hits one line in git-difftool--helper.sh > Neither does it seem to work with difftool, but I'm gonna go to bed now. > > Markus > Oops. Well, I have one final patch that removes the last bit of redundant code. It also fixed this problem so I'll go ahead and send it (it's based on top of da/difftool mentioned in pu). Since the test cases didn't catch that breakage I added a test for it. Look for a patch called: mergetool--lib: consolidate the last redundant bits in {diff,merge}tool -- David ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: git-{diff,merge} refactor round 2 2009-04-05 3:34 ` David Aguilar @ 2009-04-05 9:45 ` Junio C Hamano 2009-04-05 21:15 ` David Aguilar 0 siblings, 1 reply; 26+ messages in thread From: Junio C Hamano @ 2009-04-05 9:45 UTC (permalink / raw) To: David Aguilar; +Cc: Markus Heidelberg, charles, git David Aguilar <davvid@gmail.com> writes: > On 0, Markus Heidelberg <markus.heidelberg@web.de> wrote: >> David Aguilar, 01.04.2009: >> > Here's the 2nd round of refactoring. >> >> I just noticed that mergetool.<mergetool>.path doesn't work anymore. >> git grep mergetool.*path only hits one line in git-difftool--helper.sh >> Neither does it seem to work with difftool, but I'm gonna go to bed now. >> >> Markus >> > > Oops. Well, I have one final patch that removes the last bit of > redundant code. It also fixed this problem so I'll go ahead > and send it (it's based on top of da/difftool mentioned in > pu). > > Since the test cases didn't catch that breakage I added a test > for it. > > Look for a patch called: > > mergetool--lib: consolidate the last redundant bits in {diff,merge}tool I'll try to queue all the outstanding da/difftool patches tonight, but I think the patches in the series are getting to the point of needing a fresh redoing. Patches like "oops, these non-user scripts should have been named with double-dash" can and should disappear. Currently they are: $ git log --oneline next..da/difftool 736e6b6 mergetool--lib: add new merge tool TortoiseMerge b3ef7cc mergetool--lib: make (g)vimdiff workable under Windows c4d690e mergetool--lib: consolidate the last redundant bits in {diff,merge}tool def88c8 mergetool-lib: specialize opendiff options when in diff mode bd52fab mergetool-lib: refactor run_mergetool and check_unchanged e87266c bash completion: add git-difftool 04c3b54 {diff,merge}tool: rename helpers to remove them from tab-completion 2a83022 mergetool-lib: add diffuse as merge and diff tool 73c59d9 mergetool-lib: specialize xxdiff options when in diff mode 273e7a2 mergetool-lib: specialize kdiff3 options when in diff mode 99511d8 mergetool: use run_mergetool from git-mergetool-lib 37c48c7 difftool: use run_mergetool from git-mergetool-lib 4e314b5 mergetool-lib: introduce run_mergetool 588954e difftool: use valid_tool from git-mergetool-lib 8af4556 mergetool: use valid_tool from git-mergetool-lib 72286b5 difftool: use get_mergetool_path from git-mergetool-lib d03b97f mergetool: use get_mergetool_path from git-mergetool-lib c6afc72 Add a mergetool-lib scriptlet for holding common merge tool functions 6108b75 mergetool: use $( ... ) instead of `backticks` 73786e2 difftool: add support for a difftool.prompt config variable 472ff62 difftool: add a -y shortcut for --no-prompt de2b85d difftool: use perl built-ins when testing for msys 9df990e difftool: add various git-difftool tests 8ac77f2 difftool: add git-difftool to the list of commands ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: git-{diff,merge} refactor round 2 2009-04-05 9:45 ` Junio C Hamano @ 2009-04-05 21:15 ` David Aguilar 2009-04-05 22:15 ` Markus Heidelberg 0 siblings, 1 reply; 26+ messages in thread From: David Aguilar @ 2009-04-05 21:15 UTC (permalink / raw) To: Junio C Hamano, Johannes.Schindelin Cc: Markus Heidelberg, charles, git, msysgit On 0, Junio C Hamano <gitster@pobox.com> wrote: > David Aguilar <davvid@gmail.com> writes: > > I'll try to queue all the outstanding da/difftool patches tonight, but I > think the patches in the series are getting to the point of needing a > fresh redoing. Patches like "oops, these non-user scripts should have > been named with double-dash" can and should disappear. > > Currently they are: > > $ git log --oneline next..da/difftool > 736e6b6 mergetool--lib: add new merge tool TortoiseMerge > b3ef7cc mergetool--lib: make (g)vimdiff workable under Windows > c4d690e mergetool--lib: consolidate the last redundant bits in {diff,merge}tool > def88c8 mergetool-lib: specialize opendiff options when in diff mode > bd52fab mergetool-lib: refactor run_mergetool and check_unchanged > e87266c bash completion: add git-difftool > 04c3b54 {diff,merge}tool: rename helpers to remove them from tab-completion > 2a83022 mergetool-lib: add diffuse as merge and diff tool > 73c59d9 mergetool-lib: specialize xxdiff options when in diff mode > 273e7a2 mergetool-lib: specialize kdiff3 options when in diff mode > 99511d8 mergetool: use run_mergetool from git-mergetool-lib > 37c48c7 difftool: use run_mergetool from git-mergetool-lib > 4e314b5 mergetool-lib: introduce run_mergetool > 588954e difftool: use valid_tool from git-mergetool-lib > 8af4556 mergetool: use valid_tool from git-mergetool-lib > 72286b5 difftool: use get_mergetool_path from git-mergetool-lib > d03b97f mergetool: use get_mergetool_path from git-mergetool-lib > c6afc72 Add a mergetool-lib scriptlet for holding common merge tool functions > 6108b75 mergetool: use $( ... ) instead of `backticks` > 73786e2 difftool: add support for a difftool.prompt config variable > 472ff62 difftool: add a -y shortcut for --no-prompt > de2b85d difftool: use perl built-ins when testing for msys > 9df990e difftool: add various git-difftool tests > 8ac77f2 difftool: add git-difftool to the list of commands It goes back even farther... d3b8cec difftool: move 'git-difftool' out of contrib d3db8cec is currently sitting in 'next' and is where the "oops, I should have used double-dash" lack of foresight began. I like the *result* of what's currently sitting in da/difftool, so rewriting history is easy now that we know exactly where we're going. I think we have a couple of options here. I won't be able to get to it until sometime tonight, but I'll throw my plan out here to get a feel for what's the better way to do it. I think we have a couple of options. I'm open to any of these: 1. Base it on the current master, completely throwing away the existing da/difftool branch. That would include throwing away the commit that's in next if we really want to be clean about the history. In the process, move Markus' mergetool fixes for windows to the top so that they can be applied independently if necessary. This series would then depend on them. This would probably mean a merge conflict with next at some point. 2. Base the series on next, keeping the 'move out of contrib' patch intact. That'll minimize conflicts but will have an extra commit that renames difftool-helper. I can still push the fixes-for-windows patches up to the top so that it makes it easier on msysgit since we already have those two patches in the msysgit 'tortoisemerge' branch. 3. Any others? Regardless of which it's based on, it's obvious that there'll be some squashing going on. Tentatively, Will be squashed: 588954e difftool: use valid_tool from git-mergetool-lib 8af4556 mergetool: use valid_tool from git-mergetool-lib Will be squashed: 72286b5 difftool: use get_mergetool_path from git-mergetool-lib d03b97f mergetool: use get_mergetool_path from git-mergetool-lib c6afc72 Add a mergetool-lib scriptlet for holding common merge tool functions Will be squashed: 99511d8 mergetool: use run_mergetool from git-mergetool-lib 37c48c7 difftool: use run_mergetool from git-mergetool-lib 4e314b5 mergetool-lib: introduce run_mergetool Will be squashed: def88c8 mergetool-lib: specialize opendiff options when in diff mode 73c59d9 mergetool-lib: specialize xxdiff options when in diff mode 273e7a2 mergetool-lib: specialize kdiff3 options when in diff mode I'll go with whatever you guys think makes the series easiest to manage. I think my preference would be to resend the entires series based on 'master', but I don't want to make it any harder to manage 'next'. Thoughts? -- David ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: git-{diff,merge} refactor round 2 2009-04-05 21:15 ` David Aguilar @ 2009-04-05 22:15 ` Markus Heidelberg 2009-04-06 0:33 ` Junio C Hamano 0 siblings, 1 reply; 26+ messages in thread From: Markus Heidelberg @ 2009-04-05 22:15 UTC (permalink / raw) To: David Aguilar; +Cc: Junio C Hamano, Johannes.Schindelin, charles, git, msysgit David Aguilar, 05.04.2009: > On 0, Junio C Hamano <gitster@pobox.com> wrote: > > David Aguilar <davvid@gmail.com> writes: > > > > I'll try to queue all the outstanding da/difftool patches tonight, but I > > think the patches in the series are getting to the point of needing a > > fresh redoing. Patches like "oops, these non-user scripts should have > > been named with double-dash" can and should disappear. > > > > Currently they are: > > > > $ git log --oneline next..da/difftool > > [..] > > It goes back even farther... > > d3b8cec difftool: move 'git-difftool' out of contrib > > d3db8cec is currently sitting in 'next' and is where the > "oops, I should have used double-dash" lack of foresight > began. > 1. Base it on the current master, completely throwing away > the existing da/difftool branch. That would include throwing > away the commit that's in next if we really want to be clean > about the history. In the process, move Markus' mergetool > fixes for windows to the top so that they can be applied > independently if necessary. This series would then depend > on them. This is my favourite, too. Additionally, what about basing these on master as well? They also are unrelated to the refactoring: def88c8 mergetool-lib: specialize opendiff options when in diff mode 2a83022 mergetool-lib: add diffuse as merge and diff tool 73c59d9 mergetool-lib: specialize xxdiff options when in diff mode 273e7a2 mergetool-lib: specialize kdiff3 options when in diff mode And start refactoring on top of these? Then these could go into master or next, since they are mostly bugfixes and the refactoring can start in pu. > This would probably mean a merge conflict with next at some > point. Revert "d3b8cec difftool: move 'git-difftool' out of contrib" from next? > Regardless of which it's based on, it's obvious that there'll > be some squashing going on. Tentatively, > > Will be squashed: > 588954e difftool: use valid_tool from git-mergetool-lib > 8af4556 mergetool: use valid_tool from git-mergetool-lib > > Will be squashed: > 72286b5 difftool: use get_mergetool_path from git-mergetool-lib > d03b97f mergetool: use get_mergetool_path from git-mergetool-lib > c6afc72 Add a mergetool-lib scriptlet for holding common merge tool functions > > Will be squashed: > 99511d8 mergetool: use run_mergetool from git-mergetool-lib > 37c48c7 difftool: use run_mergetool from git-mergetool-lib > 4e314b5 mergetool-lib: introduce run_mergetool > > Will be squashed: > def88c8 mergetool-lib: specialize opendiff options when in diff mode > 73c59d9 mergetool-lib: specialize xxdiff options when in diff mode > 273e7a2 mergetool-lib: specialize kdiff3 options when in diff mode Yes, some squashing would be nice. Similar commit messages are confusing when reading the history. Markus ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: git-{diff,merge} refactor round 2 2009-04-05 22:15 ` Markus Heidelberg @ 2009-04-06 0:33 ` Junio C Hamano 0 siblings, 0 replies; 26+ messages in thread From: Junio C Hamano @ 2009-04-06 0:33 UTC (permalink / raw) To: markus.heidelberg Cc: David Aguilar, Johannes.Schindelin, charles, git, msysgit Markus Heidelberg <markus.heidelberg@web.de> writes: > David Aguilar, 05.04.2009: > ... >> 1. Base it on the current master, completely throwing away >> the existing da/difftool branch. That would include throwing >> away the commit that's in next if we really want to be clean >> about the history. In the process, move Markus' mergetool >> fixes for windows to the top so that they can be applied >> independently if necessary. This series would then depend >> on them. > > This is my favourite, too. Ok. You two forgot another obvious option of not doing anything, but since both of you seem to be Ok with the rewrite, let's take that approach. It is easy to revert the one premature merge out of next and then merge the cleaned-up version. > Yes, some squashing would be nice. Similar commit messages are confusing > when reading the history. Agreed. ^ permalink raw reply [flat|nested] 26+ messages in thread
end of thread, other threads:[~2009-04-06 0:35 UTC | newest]
Thread overview: 26+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2009-04-01 12:55 git-{diff,merge} refactor round 2 David Aguilar
2009-04-01 12:55 ` [PATCH 01/10] difftool: add support for a difftool.prompt config variable David Aguilar
2009-04-01 12:55 ` [PATCH 02/10] mergetool: use $( ... ) instead of `backticks` David Aguilar
2009-04-01 12:55 ` [PATCH 03/10] Add a mergetool-lib scriptlet for holding common merge tool functions David Aguilar
2009-04-01 12:55 ` [PATCH 04/10] mergetool: use get_mergetool_path from git-mergetool-lib David Aguilar
2009-04-01 12:55 ` [PATCH 05/10] difftool: " David Aguilar
2009-04-01 12:55 ` [PATCH 06/10] mergetool: use valid_tool " David Aguilar
2009-04-01 12:55 ` [PATCH 07/10] difftool: " David Aguilar
2009-04-01 12:55 ` [PATCH 08/10] mergetool-lib: introduce run_mergetool David Aguilar
2009-04-01 12:55 ` [PATCH 09/10] difftool: use run_mergetool from git-mergetool-lib David Aguilar
2009-04-01 12:55 ` [PATCH 10/10] mergetool: " David Aguilar
2009-04-01 22:54 ` Markus Heidelberg
2009-04-02 20:02 ` Charles Bailey
2009-04-02 20:13 ` Markus Heidelberg
2009-04-02 20:16 ` Markus Heidelberg
2009-04-03 1:54 ` David Aguilar
2009-04-01 22:47 ` [PATCH 08/10] mergetool-lib: introduce run_mergetool Markus Heidelberg
2009-04-01 22:39 ` [PATCH 03/10] Add a mergetool-lib scriptlet for holding common merge tool functions Markus Heidelberg
2009-04-02 3:58 ` David Aguilar
2009-04-02 19:59 ` git-{diff,merge} refactor round 2 Charles Bailey
2009-04-05 2:58 ` Markus Heidelberg
2009-04-05 3:34 ` David Aguilar
2009-04-05 9:45 ` Junio C Hamano
2009-04-05 21:15 ` David Aguilar
2009-04-05 22:15 ` Markus Heidelberg
2009-04-06 0:33 ` Junio C Hamano
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).