Hi David, thanks for the comments.
I'm not really sure why I did the double-negative thing... It seems
obvious that it should be the other way around. I also unrolled the
loops and wrote a gui_mode function.
Good suggestions!
---
Changes since v4:
* Remove double-negative
* Change double-nested search loop into one for-loop
* Create gui_mode function
* Change an instance of "exclusive" to "mutually exclusive"
Changes since v3:
* Move nested for into a subshell so that IFS does not leak out of
function
Changes since v2:
* Unsuppress output in t7610
* Make `get_merge_tool` return 1 on guessed so we don't have to deal
with parsing '$guessed:$merge_tool'
Changes since v1:
* Introduce get_merge_tool_guessed function instead of changing
get_merge_tool
* Remove unnecessary if-tower in mutual exclusivity logic
Denton Liu (7):
t7610: unsuppress output
t7610: add mergetool --gui tests
mergetool: use get_merge_tool function
mergetool--lib: create gui_mode function
mergetool: fallback to tool when guitool unavailable
difftool: make --gui, --tool and --extcmd mutually exclusive
difftool: fallback on merge.guitool
Documentation/git-difftool.txt | 4 +-
Documentation/git-mergetool--lib.txt | 4 +-
Documentation/git-mergetool.txt | 4 +-
builtin/difftool.c | 13 +--
git-difftool--helper.sh | 2 +-
git-mergetool--lib.sh | 47 ++++++--
git-mergetool.sh | 12 +-
t/t7610-mergetool.sh | 163 +++++++++++++++++----------
t/t7800-difftool.sh | 24 ++++
9 files changed, 180 insertions(+), 93 deletions(-)
Range-diff against v4:
1: 919aa32e20 = 1: 9f9922cab3 t7610: unsuppress output
2: 9a1bb60b20 = 2: 0f632ca6bf t7610: add mergetool --gui tests
3: a900ce2a6a ! 3: 81dd25d8e2 mergetool: use get_merge_tool function
@@ -18,8 +18,9 @@
This change is not completely backwards compatible as there may be
external users of git-mergetool--lib. However, only one user,
- git-diffall[1], was found from searching GitHub and Google. It seems
- very unlikely that there exists an external caller that would take into
+ git-diffall[1], was found from searching GitHub and Google, and this
+ tool is superseded by `git difftool --dir-diff` anyway. It seems very
+ unlikely that there exists an external caller that would take into
account the return code of `get_merge_tool` as it would always return 0
before this change so this change probably does not affect any external
users.
@@ -63,7 +64,7 @@
}
get_merge_tool () {
-+ not_guessed=true
++ is_guessed=false
# Check if a merge tool has been configured
- merge_tool=$(get_configured_merge_tool)
+ merge_tool=$(get_configured_merge_tool $GIT_MERGETOOL_GUI)
@@ -71,10 +72,10 @@
if test -z "$merge_tool"
then
merge_tool=$(guess_merge_tool) || exit
-+ not_guessed=false
++ is_guessed=true
fi
echo "$merge_tool"
-+ test "$not_guessed" = true
++ test "$is_guessed" = false
}
mergetool_find_win32_cmd () {
-: ---------- > 4: 27a59e1e27 mergetool--lib: create gui_mode function
4: abcf91688a ! 5: 40413dbda1 mergetool: fallback to tool when guitool unavailable
@@ -15,8 +15,41 @@
3. diff.tool
4. merge.tool
- Note that the behavior for when difftool or mergetool are called without
- `--gui` should be identical with or without this patch.
+ The behavior for when difftool or mergetool are called without `--gui`
+ should be identical with or without this patch.
+
+ Note that the search loop could be written as
+
+ sections="merge"
+ keys="tool"
+ if diff_mode
+ then
+ sections="diff $sections"
+ fi
+ if gui_mode
+ then
+ keys="guitool $keys"
+ fi
+
+ merge_tool=$(
+ IFS=' '
+ for key in $keys
+ do
+ for section in $sections
+ do
+ selected=$(git config $section.$key)
+ if test -n "$selected"
+ then
+ echo "$selected"
+ return
+ fi
+ done
+ done)
+
+ which would make adding a mode in the future much easier. However,
+ adding a new mode will likely never happen as it is highly discouraged
+ so, as a result, it is written in its current form so that it's
+ immediately obvious for future readers.
Signed-off-by: Denton Liu <liu.denton@xxxxxxxxx>
@@ -42,41 +75,43 @@
}
get_configured_merge_tool () {
-- # If first argument is true, find the guitool instead
-- if test "$1" = true
-+ is_gui="$1"
-+ sections="merge"
-+ keys="tool"
-+
-+ if diff_mode
- then
+- if gui_mode
+- then
- gui_prefix=gui
-+ sections="diff $sections"
- fi
-
+- fi
+-
- # Diff mode first tries diff.(gui)tool and falls back to merge.(gui)tool.
- # Merge mode only checks merge.(gui)tool
-- if diff_mode
-+ if "$is_gui" = true
++ keys=
+ if diff_mode
then
- merge_tool=$(git config diff.${gui_prefix}tool || git config merge.${gui_prefix}tool)
-- else
++ if gui_mode
++ then
++ keys="diff.guitool merge.guitool diff.tool merge.tool"
++ else
++ keys="diff.tool merge.tool"
++ fi
+ else
- merge_tool=$(git config merge.${gui_prefix}tool)
-+ keys="guitool $keys"
++ if gui_mode
++ then
++ keys="merge.guitool merge.tool"
++ else
++ keys="merge.tool"
++ fi
fi
+
+ merge_tool=$(
+ IFS=' '
+ for key in $keys
+ do
-+ for section in $sections
-+ do
-+ if selected=$(git config $section.$key)
-+ then
-+ echo "$selected"
-+ return
-+ fi
-+ done
++ selected=$(git config $key)
++ if test -n "$selected"
++ then
++ echo "$selected"
++ return
++ fi
+ done)
+
if test -n "$merge_tool" && ! valid_tool "$merge_tool"
5: 9ec39c5af0 ! 6: c70789b689 difftool: make --gui, --tool and --extcmd mutually exclusive
@@ -29,7 +29,7 @@
test_cmp expect actual
'
-+test_expect_success 'difftool --gui, --tool and --extcmd are exclusive' '
++test_expect_success 'difftool --gui, --tool and --extcmd are mutually exclusive' '
+ difftool_test_setup &&
+ test_must_fail git difftool --gui --tool=test-tool &&
+ test_must_fail git difftool --gui --extcmd=cat &&
6: a72009fc3d = 7: 3fd4f46a7c difftool: fallback on merge.guitool
--
2.21.0.1033.g0e8cc1100c
