diff mbox series

[v3,3/6] mergetool: use get_merge_tool function

Message ID ff83d25ff2d0b9585b52c7e28cd7b93292b5304d.1556142510.git.liu.denton@gmail.com (mailing list archive)
State New, archived
Headers show
Series difftool and mergetool improvements | expand

Commit Message

Denton Liu April 24, 2019, 10:47 p.m. UTC
In git-mergetool, the logic for getting which merge tool to use is
duplicated in git-mergetool--lib, except for the fact that it needs to
know whether the tool was guessed or not.

Rewrite `get_merge_tool` to return whether or not the tool was guessed
through the return code and make git-mergetool call this function
instead of duplicating the logic. Note that 1 was chosen to be the
return code of when a tool is guessed because it seems like a slightly
more abnormal condition than getting a tool that's explicitly specified
but this is completely arbitrary.

Also, let `$GIT_MERGETOOL_GUI` be set to determine whether or not the
guitool will be selected.

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
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.

[1]: https://github.com/thenigan/git-diffall

Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
 Documentation/git-mergetool--lib.txt |  4 +++-
 git-difftool--helper.sh              |  2 +-
 git-mergetool--lib.sh                |  5 ++++-
 git-mergetool.sh                     | 12 ++++--------
 4 files changed, 12 insertions(+), 11 deletions(-)

Comments

Junio C Hamano April 25, 2019, 2:36 a.m. UTC | #1
Denton Liu <liu.denton@gmail.com> writes:

> 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
> 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.

Sounds like a risk that is OK to accept, as it is easy enough to
reintroduce the middle layer function like you did in the previous
round when it becomes an issue.

Will queue; thanks.
diff mbox series

Patch

diff --git a/Documentation/git-mergetool--lib.txt b/Documentation/git-mergetool--lib.txt
index 055550b2bc..4da9d24096 100644
--- a/Documentation/git-mergetool--lib.txt
+++ b/Documentation/git-mergetool--lib.txt
@@ -28,7 +28,9 @@  to define the operation mode for the functions listed below.
 FUNCTIONS
 ---------
 get_merge_tool::
-	returns a merge tool.
+	returns a merge tool. the return code is 1 if we returned a guessed
+	merge tool, else 0. '$GIT_MERGETOOL_GUI' may be set to 'true' to
+	search for the appropriate guitool.
 
 get_merge_tool_cmd::
 	returns the custom command for a merge tool.
diff --git a/git-difftool--helper.sh b/git-difftool--helper.sh
index 7bfb6737df..46af3e60b7 100755
--- a/git-difftool--helper.sh
+++ b/git-difftool--helper.sh
@@ -71,7 +71,7 @@  then
 	then
 		merge_tool="$GIT_DIFF_TOOL"
 	else
-		merge_tool="$(get_merge_tool)" || exit
+		merge_tool="$(get_merge_tool)"
 	fi
 fi
 
diff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh
index 83bf52494c..68ff26a0f7 100644
--- a/git-mergetool--lib.sh
+++ b/git-mergetool--lib.sh
@@ -403,14 +403,17 @@  get_merge_tool_path () {
 }
 
 get_merge_tool () {
+	not_guessed=true
 	# Check if a merge tool has been configured
-	merge_tool=$(get_configured_merge_tool)
+	merge_tool=$(get_configured_merge_tool $GIT_MERGETOOL_GUI)
 	# Try to guess an appropriate merge tool if no tool has been set.
 	if test -z "$merge_tool"
 	then
 		merge_tool=$(guess_merge_tool) || exit
+		not_guessed=false
 	fi
 	echo "$merge_tool"
+	test "$not_guessed" = true
 }
 
 mergetool_find_win32_cmd () {
diff --git a/git-mergetool.sh b/git-mergetool.sh
index 01b9ad59b2..88fa6a914a 100755
--- a/git-mergetool.sh
+++ b/git-mergetool.sh
@@ -389,7 +389,7 @@  print_noop_and_exit () {
 
 main () {
 	prompt=$(git config --bool mergetool.prompt)
-	gui_tool=false
+	GIT_MERGETOOL_GUI=false
 	guessed_merge_tool=false
 	orderfile=
 
@@ -416,10 +416,10 @@  main () {
 			esac
 			;;
 		--no-gui)
-			gui_tool=false
+			GIT_MERGETOOL_GUI=false
 			;;
 		-g|--gui)
-			gui_tool=true
+			GIT_MERGETOOL_GUI=true
 			;;
 		-y|--no-prompt)
 			prompt=false
@@ -449,12 +449,8 @@  main () {
 
 	if test -z "$merge_tool"
 	then
-		# Check if a merge tool has been configured
-		merge_tool=$(get_configured_merge_tool $gui_tool)
-		# Try to guess an appropriate merge tool if no tool has been set.
-		if test -z "$merge_tool"
+		if ! merge_tool=$(get_merge_tool)
 		then
-			merge_tool=$(guess_merge_tool) || exit
 			guessed_merge_tool=true
 		fi
 	fi