git.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/5] difftool and mergetool improvements
@ 2019-04-22  5:07 Denton Liu
  2019-04-22  5:07 ` [PATCH 1/5] t7610: add mergetool --gui tests Denton Liu
                   ` (5 more replies)
  0 siblings, 6 replies; 48+ messages in thread
From: Denton Liu @ 2019-04-22  5:07 UTC (permalink / raw)
  To: Git Mailing List; +Cc: Johannes Schindelin, David Aguilar

I noticed earlier that when running 'git difftool --gui', we do not
fallback to 'merge.guitool' when 'diff.guitool' isn't set, even though
this behaviour exists for when we invoke 'git difftool' (i.e. it falls
back from diff.tool to merge.tool).

While fixing this bug up, I noticed a few other places where we could do
some code cleanup/add tests so I did that as well.

Denton Liu (5):
  t7610: add mergetool --gui tests
  mergetool: use get_merge_tool function
  mergetool: fallback to tool when guitool unavailable
  difftool: make --gui, --tool and --extcmd exclusive
  difftool: fallback on merge.guitool

 Documentation/git-difftool.txt       |  4 ++-
 Documentation/git-mergetool--lib.txt |  5 +++-
 Documentation/git-mergetool.txt      |  4 ++-
 builtin/difftool.c                   | 21 ++++++++------
 git-difftool--helper.sh              |  2 +-
 git-mergetool--lib.sh                | 33 ++++++++++++++--------
 git-mergetool.sh                     | 11 ++------
 t/t7610-mergetool.sh                 | 41 ++++++++++++++++++++++++++++
 t/t7800-difftool.sh                  | 24 ++++++++++++++++
 9 files changed, 113 insertions(+), 32 deletions(-)

-- 
2.21.0.967.gf85e14fd49


^ permalink raw reply	[flat|nested] 48+ messages in thread

* [PATCH 1/5] t7610: add mergetool --gui tests
  2019-04-22  5:07 [PATCH 0/5] difftool and mergetool improvements Denton Liu
@ 2019-04-22  5:07 ` Denton Liu
  2019-04-22  5:07 ` [PATCH 2/5] mergetool: use get_merge_tool function Denton Liu
                   ` (4 subsequent siblings)
  5 siblings, 0 replies; 48+ messages in thread
From: Denton Liu @ 2019-04-22  5:07 UTC (permalink / raw)
  To: Git Mailing List; +Cc: Johannes Schindelin, David Aguilar

In 063f2bdbf7 (mergetool: accept -g/--[no-]gui as arguments,
2018-10-24), mergetool was taught the --gui option but no tests were
added to ensure that it was working properly. Add a test to ensure that
it works.

Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
 t/t7610-mergetool.sh | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/t/t7610-mergetool.sh b/t/t7610-mergetool.sh
index a9fb971615..5f37d7a1ff 100755
--- a/t/t7610-mergetool.sh
+++ b/t/t7610-mergetool.sh
@@ -145,6 +145,28 @@ test_expect_success 'custom mergetool' '
 	git commit -m "branch1 resolved with mergetool"
 '
 
+test_expect_success 'gui mergetool' '
+	test_config merge.guitool myguitool &&
+	test_config mergetool.myguitool.cmd "(printf \"gui \" && cat \"\$REMOTE\") >\"\$MERGED\"" &&
+	test_config mergetool.myguitool.trustExitCode true &&
+	test_when_finished "git reset --hard" &&
+	git checkout -b test$test_count branch1 &&
+	git submodule update -N &&
+	test_must_fail git merge master >/dev/null 2>&1 &&
+	( yes "" | git mergetool --gui both >/dev/null 2>&1 ) &&
+	( yes "" | git mergetool -g file1 file1 ) &&
+	( yes "" | git mergetool --gui file2 "spaced name" >/dev/null 2>&1 ) &&
+	( yes "" | git mergetool --gui subdir/file3 >/dev/null 2>&1 ) &&
+	( yes "d" | git mergetool --gui file11 >/dev/null 2>&1 ) &&
+	( yes "d" | git mergetool --gui file12 >/dev/null 2>&1 ) &&
+	( yes "l" | git mergetool --gui submod >/dev/null 2>&1 ) &&
+	test "$(cat file1)" = "gui master updated" &&
+	test "$(cat file2)" = "gui master new" &&
+	test "$(cat subdir/file3)" = "gui master new sub" &&
+	test "$(cat submod/bar)" = "branch1 submodule" &&
+	git commit -m "branch1 resolved with mergetool"
+'
+
 test_expect_success 'mergetool crlf' '
 	test_when_finished "git reset --hard" &&
 	# This test_config line must go after the above reset line so that
-- 
2.21.0.967.gf85e14fd49


^ permalink raw reply related	[flat|nested] 48+ messages in thread

* [PATCH 2/5] mergetool: use get_merge_tool function
  2019-04-22  5:07 [PATCH 0/5] difftool and mergetool improvements Denton Liu
  2019-04-22  5:07 ` [PATCH 1/5] t7610: add mergetool --gui tests Denton Liu
@ 2019-04-22  5:07 ` Denton Liu
  2019-04-22  7:07   ` Eric Sunshine
  2019-04-22  5:07 ` [PATCH 3/5] mergetool: fallback to tool when guitool unavailable Denton Liu
                   ` (3 subsequent siblings)
  5 siblings, 1 reply; 48+ messages in thread
From: Denton Liu @ 2019-04-22  5:07 UTC (permalink / raw)
  To: Git Mailing List; +Cc: Johannes Schindelin, David Aguilar

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
and make git-mergetool call this function instead of duplicating the
logic. Also, let `$GIT_MERGETOOL_GUI` be set to determine whether or not
the guitool will be selected.

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

diff --git a/Documentation/git-mergetool--lib.txt b/Documentation/git-mergetool--lib.txt
index 055550b2bc..c4f10209e0 100644
--- a/Documentation/git-mergetool--lib.txt
+++ b/Documentation/git-mergetool--lib.txt
@@ -28,7 +28,10 @@ to define the operation mode for the functions listed below.
 FUNCTIONS
 ---------
 get_merge_tool::
-	returns a merge tool.
+	returns '$is_guessed:$merge_tool'. '$is_guessed' is 'true' if
+	the tool was guessed, else 'false'. '$merge_tool' is the merge
+	tool to use. '$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..78a0446668 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 | sed -e 's/^[a-z]*://')" || exit
 	fi
 fi
 
diff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh
index 83bf52494c..d5e2c6c5c6 100644
--- a/git-mergetool--lib.sh
+++ b/git-mergetool--lib.sh
@@ -403,14 +403,16 @@ get_merge_tool_path () {
 }
 
 get_merge_tool () {
+	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)
 	# 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
+		is_guessed=true
 	fi
-	echo "$merge_tool"
+	echo "$is_guessed:$merge_tool"
 }
 
 mergetool_find_win32_cmd () {
diff --git a/git-mergetool.sh b/git-mergetool.sh
index 01b9ad59b2..6ad8024e46 100755
--- a/git-mergetool.sh
+++ b/git-mergetool.sh
@@ -449,14 +449,9 @@ 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"
-		then
-			merge_tool=$(guess_merge_tool) || exit
-			guessed_merge_tool=true
-		fi
+		IFS=':' read guessed_merge_tool merge_tool <<-EOF
+		$(GIT_MERGETOOL_GUI=$gui_tool get_merge_tool)
+		EOF
 	fi
 	merge_keep_backup="$(git config --bool mergetool.keepBackup || echo true)"
 	merge_keep_temporaries="$(git config --bool mergetool.keepTemporaries || echo false)"
-- 
2.21.0.967.gf85e14fd49


^ permalink raw reply related	[flat|nested] 48+ messages in thread

* [PATCH 3/5] mergetool: fallback to tool when guitool unavailable
  2019-04-22  5:07 [PATCH 0/5] difftool and mergetool improvements Denton Liu
  2019-04-22  5:07 ` [PATCH 1/5] t7610: add mergetool --gui tests Denton Liu
  2019-04-22  5:07 ` [PATCH 2/5] mergetool: use get_merge_tool function Denton Liu
@ 2019-04-22  5:07 ` Denton Liu
  2019-04-22  5:07 ` [PATCH 4/5] difftool: make --gui, --tool and --extcmd exclusive Denton Liu
                   ` (2 subsequent siblings)
  5 siblings, 0 replies; 48+ messages in thread
From: Denton Liu @ 2019-04-22  5:07 UTC (permalink / raw)
  To: Git Mailing List; +Cc: Johannes Schindelin, David Aguilar

In git-difftool, if the tool is called with --gui but `diff.guitool` is
not set, it falls back to `diff.tool`. Make git-mergetool also fallback
from `merge.guitool` to `merge.tool` if the former is undefined.

If git-difftool were to use `get_configured_mergetool`, it would also
get the fallback behaviour in the following precedence:

1. diff.guitool
2. merge.guitool
3. diff.tool
4. merge.tool

Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
 Documentation/git-mergetool.txt |  4 +++-
 git-mergetool--lib.sh           | 27 ++++++++++++++++++---------
 t/t7610-mergetool.sh            | 19 +++++++++++++++++++
 3 files changed, 40 insertions(+), 10 deletions(-)

diff --git a/Documentation/git-mergetool.txt b/Documentation/git-mergetool.txt
index 0c7975a050..6b14702e78 100644
--- a/Documentation/git-mergetool.txt
+++ b/Documentation/git-mergetool.txt
@@ -83,7 +83,9 @@ success of the resolution after the custom tool has exited.
 --gui::
 	When 'git-mergetool' is invoked with the `-g` or `--gui` option
 	the default merge tool will be read from the configured
-	`merge.guitool` variable instead of `merge.tool`.
+	`merge.guitool` variable instead of `merge.tool`. If
+	`merge.guitool` is not set, we will fallback to the tool
+	configured under `merge.tool`.
 
 --no-gui::
 	This overrides a previous `-g` or `--gui` setting and reads the
diff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh
index d5e2c6c5c6..68a85f4a7b 100644
--- a/git-mergetool--lib.sh
+++ b/git-mergetool--lib.sh
@@ -350,20 +350,29 @@ guess_merge_tool () {
 }
 
 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
-		gui_prefix=gui
+		sections="diff $sections"
 	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
 	then
-		merge_tool=$(git config diff.${gui_prefix}tool || git config merge.${gui_prefix}tool)
-	else
-		merge_tool=$(git config merge.${gui_prefix}tool)
+		keys="guitool $keys"
 	fi
+
+	IFS=' '
+	for key in $keys
+	do
+		for section in $sections
+		do
+			merge_tool=$(git config $section.$key) && break 2
+		done
+	done
+
 	if test -n "$merge_tool" && ! valid_tool "$merge_tool"
 	then
 		echo >&2 "git config option $TOOL_MODE.${gui_prefix}tool set to unknown tool: $merge_tool"
diff --git a/t/t7610-mergetool.sh b/t/t7610-mergetool.sh
index 5f37d7a1ff..bc2c9eaa30 100755
--- a/t/t7610-mergetool.sh
+++ b/t/t7610-mergetool.sh
@@ -167,6 +167,25 @@ test_expect_success 'gui mergetool' '
 	git commit -m "branch1 resolved with mergetool"
 '
 
+test_expect_success 'gui mergetool without merge.guitool set fallsback to merge.tool' '
+	test_when_finished "git reset --hard" &&
+	git checkout -b test$test_count branch1 &&
+	git submodule update -N &&
+	test_must_fail git merge master >/dev/null 2>&1 &&
+	( yes "" | git mergetool --gui both >/dev/null 2>&1 ) &&
+	( yes "" | git mergetool -g file1 file1 ) &&
+	( yes "" | git mergetool --gui file2 "spaced name" >/dev/null 2>&1 ) &&
+	( yes "" | git mergetool --gui subdir/file3 >/dev/null 2>&1 ) &&
+	( yes "d" | git mergetool --gui file11 >/dev/null 2>&1 ) &&
+	( yes "d" | git mergetool --gui file12 >/dev/null 2>&1 ) &&
+	( yes "l" | git mergetool --gui submod >/dev/null 2>&1 ) &&
+	test "$(cat file1)" = "master updated" &&
+	test "$(cat file2)" = "master new" &&
+	test "$(cat subdir/file3)" = "master new sub" &&
+	test "$(cat submod/bar)" = "branch1 submodule" &&
+	git commit -m "branch1 resolved with mergetool"
+'
+
 test_expect_success 'mergetool crlf' '
 	test_when_finished "git reset --hard" &&
 	# This test_config line must go after the above reset line so that
-- 
2.21.0.967.gf85e14fd49


^ permalink raw reply related	[flat|nested] 48+ messages in thread

* [PATCH 4/5] difftool: make --gui, --tool and --extcmd exclusive
  2019-04-22  5:07 [PATCH 0/5] difftool and mergetool improvements Denton Liu
                   ` (2 preceding siblings ...)
  2019-04-22  5:07 ` [PATCH 3/5] mergetool: fallback to tool when guitool unavailable Denton Liu
@ 2019-04-22  5:07 ` Denton Liu
  2019-04-22  7:03   ` Eric Sunshine
  2019-04-22  5:07 ` [PATCH 5/5] difftool: fallback on merge.guitool Denton Liu
  2019-04-23  8:53 ` [PATCH v2 0/5] difftool and mergetool improvements Denton Liu
  5 siblings, 1 reply; 48+ messages in thread
From: Denton Liu @ 2019-04-22  5:07 UTC (permalink / raw)
  To: Git Mailing List; +Cc: Johannes Schindelin, David Aguilar

In git-difftool, these options specify which tool to ultimately run. As
a result, they are logically conflicting. Explicitly disallow these
options from being used together.

Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
 builtin/difftool.c  | 11 ++++++++++-
 t/t7800-difftool.sh |  8 ++++++++
 2 files changed, 18 insertions(+), 1 deletion(-)

diff --git a/builtin/difftool.c b/builtin/difftool.c
index a3ea60ea71..5ad39c9172 100644
--- a/builtin/difftool.c
+++ b/builtin/difftool.c
@@ -690,7 +690,7 @@ static int run_file_diff(int prompt, const char *prefix,
 int cmd_difftool(int argc, const char **argv, const char *prefix)
 {
 	int use_gui_tool = 0, dir_diff = 0, prompt = -1, symlinks = 0,
-	    tool_help = 0;
+	    tool_help = 0, count = 0;
 	static char *difftool_cmd = NULL, *extcmd = NULL;
 	struct option builtin_difftool_options[] = {
 		OPT_BOOL('g', "gui", &use_gui_tool,
@@ -731,6 +731,15 @@ int cmd_difftool(int argc, const char **argv, const char *prefix)
 	setenv(GIT_DIR_ENVIRONMENT, absolute_path(get_git_dir()), 1);
 	setenv(GIT_WORK_TREE_ENVIRONMENT, absolute_path(get_git_work_tree()), 1);
 
+	if (use_gui_tool)
+		count++;
+	if (difftool_cmd)
+		count++;
+	if (extcmd)
+		count++;
+	if (count > 1)
+		die(_("--gui, --tool and --extcmd are exclusive"));
+
 	if (use_gui_tool && diff_gui_tool && *diff_gui_tool)
 		setenv("GIT_DIFF_TOOL", diff_gui_tool, 1);
 	else if (difftool_cmd) {
diff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh
index bb9a7f4ff9..107f31213d 100755
--- a/t/t7800-difftool.sh
+++ b/t/t7800-difftool.sh
@@ -705,4 +705,12 @@ test_expect_success SYMLINKS 'difftool --dir-diff handles modified symlinks' '
 	test_cmp expect actual
 '
 
+test_expect_success 'difftool --gui, --tool and --extcmd are exclusive' '
+	difftool_test_setup &&
+	test_must_fail git difftool --gui --tool=test-tool &&
+	test_must_fail git difftool --gui --extcmd=cat &&
+	test_must_fail git difftool --tool=test-tool --extcmd=cat &&
+	test_must_fail git difftool --gui --tool=test-tool --extcmd=cat
+'
+
 test_done
-- 
2.21.0.967.gf85e14fd49


^ permalink raw reply related	[flat|nested] 48+ messages in thread

* [PATCH 5/5] difftool: fallback on merge.guitool
  2019-04-22  5:07 [PATCH 0/5] difftool and mergetool improvements Denton Liu
                   ` (3 preceding siblings ...)
  2019-04-22  5:07 ` [PATCH 4/5] difftool: make --gui, --tool and --extcmd exclusive Denton Liu
@ 2019-04-22  5:07 ` Denton Liu
  2019-04-22 18:18   ` Jeff Hostetler
  2019-04-23  8:53 ` [PATCH v2 0/5] difftool and mergetool improvements Denton Liu
  5 siblings, 1 reply; 48+ messages in thread
From: Denton Liu @ 2019-04-22  5:07 UTC (permalink / raw)
  To: Git Mailing List; +Cc: Johannes Schindelin, David Aguilar

In git-difftool.txt, it says

	'git difftool' falls back to 'git mergetool' config variables when the
	difftool equivalents have not been defined.

However, when `diff.guitool` is missing, it doesn't fallback to
anything. Make git-difftool fallback to `merge.guitool` when `diff.guitool` is
missing.

Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
 Documentation/git-difftool.txt |  4 +++-
 builtin/difftool.c             | 10 ++--------
 t/t7800-difftool.sh            | 16 ++++++++++++++++
 3 files changed, 21 insertions(+), 9 deletions(-)

diff --git a/Documentation/git-difftool.txt b/Documentation/git-difftool.txt
index 96c26e6aa8..484c485fd0 100644
--- a/Documentation/git-difftool.txt
+++ b/Documentation/git-difftool.txt
@@ -90,7 +90,9 @@ instead.  `--no-symlinks` is the default on Windows.
 	When 'git-difftool' is invoked with the `-g` or `--gui` option
 	the default diff tool will be read from the configured
 	`diff.guitool` variable instead of `diff.tool`. The `--no-gui`
-	option can be used to override this setting.
+	option can be used to override this setting. If `diff.guitool`
+	is not set, we will fallback in the order of `merge.guitool`,
+	`diff.tool`, `merge.tool` until a tool is found.
 
 --[no-]trust-exit-code::
 	'git-difftool' invokes a diff tool individually on each file.
diff --git a/builtin/difftool.c b/builtin/difftool.c
index 5ad39c9172..67f26502c5 100644
--- a/builtin/difftool.c
+++ b/builtin/difftool.c
@@ -24,7 +24,6 @@
 #include "object-store.h"
 #include "dir.h"
 
-static char *diff_gui_tool;
 static int trust_exit_code;
 
 static const char *const builtin_difftool_usage[] = {
@@ -34,11 +33,6 @@ static const char *const builtin_difftool_usage[] = {
 
 static int difftool_config(const char *var, const char *value, void *cb)
 {
-	if (!strcmp(var, "diff.guitool")) {
-		diff_gui_tool = xstrdup(value);
-		return 0;
-	}
-
 	if (!strcmp(var, "difftool.trustexitcode")) {
 		trust_exit_code = git_config_bool(var, value);
 		return 0;
@@ -740,8 +734,8 @@ int cmd_difftool(int argc, const char **argv, const char *prefix)
 	if (count > 1)
 		die(_("--gui, --tool and --extcmd are exclusive"));
 
-	if (use_gui_tool && diff_gui_tool && *diff_gui_tool)
-		setenv("GIT_DIFF_TOOL", diff_gui_tool, 1);
+	if (use_gui_tool)
+		setenv("GIT_MERGETOOL_GUI", "true", 1);
 	else if (difftool_cmd) {
 		if (*difftool_cmd)
 			setenv("GIT_DIFF_TOOL", difftool_cmd, 1);
diff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh
index 107f31213d..ae90701a12 100755
--- a/t/t7800-difftool.sh
+++ b/t/t7800-difftool.sh
@@ -279,11 +279,27 @@ test_expect_success 'difftool + mergetool config variables' '
 	echo branch >expect &&
 	git difftool --no-prompt branch >actual &&
 	test_cmp expect actual &&
+	git difftool --gui --no-prompt branch >actual &&
+	test_cmp expect actual &&
 
 	# set merge.tool to something bogus, diff.tool to test-tool
 	test_config merge.tool bogus-tool &&
 	test_config diff.tool test-tool &&
 	git difftool --no-prompt branch >actual &&
+	test_cmp expect actual &&
+	git difftool --gui --no-prompt branch >actual &&
+	test_cmp expect actual &&
+
+	# set merge.tool, diff.tool to something bogus, merge.guitool to test-tool
+	test_config diff.tool bogus-tool &&
+	test_config merge.guitool test-tool &&
+	git difftool --gui --no-prompt branch >actual &&
+	test_cmp expect actual &&
+
+	# set merge.tool, diff.tool, merge.guitool to something bogus, diff.guitool to test-tool
+	test_config merge.guitool bogus-tool &&
+	test_config diff.guitool test-tool &&
+	git difftool --gui --no-prompt branch >actual &&
 	test_cmp expect actual
 '
 
-- 
2.21.0.967.gf85e14fd49


^ permalink raw reply related	[flat|nested] 48+ messages in thread

* Re: [PATCH 4/5] difftool: make --gui, --tool and --extcmd exclusive
  2019-04-22  5:07 ` [PATCH 4/5] difftool: make --gui, --tool and --extcmd exclusive Denton Liu
@ 2019-04-22  7:03   ` Eric Sunshine
  0 siblings, 0 replies; 48+ messages in thread
From: Eric Sunshine @ 2019-04-22  7:03 UTC (permalink / raw)
  To: Denton Liu; +Cc: Git Mailing List, Johannes Schindelin, David Aguilar

On Mon, Apr 22, 2019 at 1:07 AM Denton Liu <liu.denton@gmail.com> wrote:
> In git-difftool, these options specify which tool to ultimately run. As
> a result, they are logically conflicting. Explicitly disallow these
> options from being used together.
>
> Signed-off-by: Denton Liu <liu.denton@gmail.com>
> ---
> diff --git a/builtin/difftool.c b/builtin/difftool.c
> @@ -731,6 +731,15 @@ int cmd_difftool(int argc, const char **argv, const char *prefix)
> +       if (use_gui_tool)
> +               count++;
> +       if (difftool_cmd)
> +               count++;
> +       if (extcmd)
> +               count++;
> +       if (count > 1)
> +               die(_("--gui, --tool and --extcmd are exclusive"));

The more typical way to check this condition in this codebase is:

    if (use_gui_tool + !!difftool_cmd + !!extcmd > 1)
        die(...);

Also, I think you might want: s/exclusive/mutually &/

^ permalink raw reply	[flat|nested] 48+ messages in thread

* Re: [PATCH 2/5] mergetool: use get_merge_tool function
  2019-04-22  5:07 ` [PATCH 2/5] mergetool: use get_merge_tool function Denton Liu
@ 2019-04-22  7:07   ` Eric Sunshine
  2019-04-22  8:35     ` Denton Liu
  0 siblings, 1 reply; 48+ messages in thread
From: Eric Sunshine @ 2019-04-22  7:07 UTC (permalink / raw)
  To: Denton Liu; +Cc: Git Mailing List, Johannes Schindelin, David Aguilar

On Mon, Apr 22, 2019 at 1:07 AM Denton Liu <liu.denton@gmail.com> wrote:
> [...]
> Rewrite `get_merge_tool` to return whether or not the tool was guessed
> and make git-mergetool call this function instead of duplicating the
> logic. Also, let `$GIT_MERGETOOL_GUI` be set to determine whether or not
> the guitool will be selected.
>
> Signed-off-by: Denton Liu <liu.denton@gmail.com>
> ---
> diff --git a/Documentation/git-mergetool--lib.txt b/Documentation/git-mergetool--lib.txt
> @@ -28,7 +28,10 @@ to define the operation mode for the functions listed below.
>  get_merge_tool::
> -       returns a merge tool.
> +       returns '$is_guessed:$merge_tool'. '$is_guessed' is 'true' if
> +       the tool was guessed, else 'false'. '$merge_tool' is the merge
> +       tool to use. '$GIT_MERGETOOL_GUI' may be set to 'true' to search
> +       for the appropriate guitool.

What is the likelihood that code outside of our control is using this
function? If there is such code, this backward-incompatible change
will break that code. If the likelihood is excessively small, perhaps
it is not worth worrying about, otherwise, perhaps this warrants a new
function with a distinct name.

^ permalink raw reply	[flat|nested] 48+ messages in thread

* Re: [PATCH 2/5] mergetool: use get_merge_tool function
  2019-04-22  7:07   ` Eric Sunshine
@ 2019-04-22  8:35     ` Denton Liu
  0 siblings, 0 replies; 48+ messages in thread
From: Denton Liu @ 2019-04-22  8:35 UTC (permalink / raw)
  To: Eric Sunshine; +Cc: Git Mailing List, Johannes Schindelin, David Aguilar

Hi Eric,

On Mon, Apr 22, 2019 at 03:07:25AM -0400, Eric Sunshine wrote:
> On Mon, Apr 22, 2019 at 1:07 AM Denton Liu <liu.denton@gmail.com> wrote:
> > [...]
> > Rewrite `get_merge_tool` to return whether or not the tool was guessed
> > and make git-mergetool call this function instead of duplicating the
> > logic. Also, let `$GIT_MERGETOOL_GUI` be set to determine whether or not
> > the guitool will be selected.
> >
> > Signed-off-by: Denton Liu <liu.denton@gmail.com>
> > ---
> > diff --git a/Documentation/git-mergetool--lib.txt b/Documentation/git-mergetool--lib.txt
> > @@ -28,7 +28,10 @@ to define the operation mode for the functions listed below.
> >  get_merge_tool::
> > -       returns a merge tool.
> > +       returns '$is_guessed:$merge_tool'. '$is_guessed' is 'true' if
> > +       the tool was guessed, else 'false'. '$merge_tool' is the merge
> > +       tool to use. '$GIT_MERGETOOL_GUI' may be set to 'true' to search
> > +       for the appropriate guitool.
> 
> What is the likelihood that code outside of our control is using this
> function? If there is such code, this backward-incompatible change
> will break that code. If the likelihood is excessively small, perhaps
> it is not worth worrying about, otherwise, perhaps this warrants a new
> function with a distinct name.

Thanks for considering this, I hadn't thought about it myself. I assumed
this was an internal function but I guess I was wrong.

I did a bit of digging on GitHub and Google and I found that
git-diffall[1] uses it, although it seems quite old and unmaintained.
Aside from this, I can't find any other open-source programs which use
git-mergetool--lib (and in particular, get_merge_tool) so I believe that
it is pretty rare.

That being said, I'm open to writing a new function so that the change
will be backwards compatible. I'll see what the list has to say.

Thanks,

Denton

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

^ permalink raw reply	[flat|nested] 48+ messages in thread

* Re: [PATCH 5/5] difftool: fallback on merge.guitool
  2019-04-22  5:07 ` [PATCH 5/5] difftool: fallback on merge.guitool Denton Liu
@ 2019-04-22 18:18   ` Jeff Hostetler
  2019-04-22 18:33     ` Denton Liu
  0 siblings, 1 reply; 48+ messages in thread
From: Jeff Hostetler @ 2019-04-22 18:18 UTC (permalink / raw)
  To: Denton Liu, Git Mailing List; +Cc: Johannes Schindelin, David Aguilar



On 4/22/2019 1:07 AM, Denton Liu wrote:
> In git-difftool.txt, it says
> 
> 	'git difftool' falls back to 'git mergetool' config variables when the
> 	difftool equivalents have not been defined.
> 
> However, when `diff.guitool` is missing, it doesn't fallback to
> anything. Make git-difftool fallback to `merge.guitool` when `diff.guitool` is
> missing.
> 

Is this a well-defined operation?

I mean, we're assuming here that a 3-way gui merge tool (that probably
expects 3 input pathnames and maybe a 4th merge-result pathname (and
associated titles and etc)) can function sanely when only given the
pair that a diff would have.

That is, we're assuming that the selected merge tool has a 2-way diff
mode and that the command line args for the 2- and 3-way views are
compatible.

Just a thought
Jeff



^ permalink raw reply	[flat|nested] 48+ messages in thread

* Re: [PATCH 5/5] difftool: fallback on merge.guitool
  2019-04-22 18:18   ` Jeff Hostetler
@ 2019-04-22 18:33     ` Denton Liu
  0 siblings, 0 replies; 48+ messages in thread
From: Denton Liu @ 2019-04-22 18:33 UTC (permalink / raw)
  To: Jeff Hostetler; +Cc: Git Mailing List, Johannes Schindelin, David Aguilar

Hi Jeff,

On Mon, Apr 22, 2019 at 02:18:36PM -0400, Jeff Hostetler wrote:
> 
> 
> On 4/22/2019 1:07 AM, Denton Liu wrote:
> >In git-difftool.txt, it says
> >
> >	'git difftool' falls back to 'git mergetool' config variables when the
> >	difftool equivalents have not been defined.
> >
> >However, when `diff.guitool` is missing, it doesn't fallback to
> >anything. Make git-difftool fallback to `merge.guitool` when `diff.guitool` is
> >missing.
> >
> 
> Is this a well-defined operation?

I believe this is a yes.

> 
> I mean, we're assuming here that a 3-way gui merge tool (that probably
> expects 3 input pathnames and maybe a 4th merge-result pathname (and
> associated titles and etc)) can function sanely when only given the
> pair that a diff would have.
> 
> That is, we're assuming that the selected merge tool has a 2-way diff
> mode and that the command line args for the 2- and 3-way views are
> compatible.

If I read the code correctly, it seems like the only tool that is
strictly "merge-only" is tortoisemerge. In mergetools/tortoisemerge, we
have

	can_diff () {
		return 1
	}

which means it will refuse to run as a difftool.

In the case where it fails like this, it'll loudly complain to the user,
which'll give them a chance to fix their config. I believe that this is
desired behaviour and the patch adds on top of that.

Thanks,

Denton

> 
> Just a thought
> Jeff
> 
> 

^ permalink raw reply	[flat|nested] 48+ messages in thread

* [PATCH v2 0/5] difftool and mergetool improvements
  2019-04-22  5:07 [PATCH 0/5] difftool and mergetool improvements Denton Liu
                   ` (4 preceding siblings ...)
  2019-04-22  5:07 ` [PATCH 5/5] difftool: fallback on merge.guitool Denton Liu
@ 2019-04-23  8:53 ` Denton Liu
  2019-04-23  8:53   ` [PATCH v2 1/5] t7610: add mergetool --gui tests Denton Liu
                     ` (5 more replies)
  5 siblings, 6 replies; 48+ messages in thread
From: Denton Liu @ 2019-04-23  8:53 UTC (permalink / raw)
  To: Git Mailing List
  Cc: Johannes Schindelin, David Aguilar, Jeff Hostetler, Eric Sunshine

Thanks for the review, Eric.

Hopefully, these changes have addressed the concerns that you've raised.

---

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 (5):
  t7610: add mergetool --gui tests
  mergetool: use get_merge_tool_guessed 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 |  9 +++++-
 Documentation/git-mergetool.txt      |  4 ++-
 builtin/difftool.c                   | 13 ++++-----
 git-mergetool--lib.sh                | 39 ++++++++++++++++++--------
 git-mergetool.sh                     | 11 ++------
 t/t7610-mergetool.sh                 | 41 ++++++++++++++++++++++++++++
 t/t7800-difftool.sh                  | 24 ++++++++++++++++
 8 files changed, 114 insertions(+), 31 deletions(-)

Range-diff against v1:
1:  678f9b11fc = 1:  678f9b11fc t7610: add mergetool --gui tests
2:  692875cf4b ! 2:  e928db892e mergetool: use get_merge_tool function
    @@ -1,15 +1,19 @@
     Author: Denton Liu <liu.denton@gmail.com>
     
    -    mergetool: use get_merge_tool function
    +    mergetool: use get_merge_tool_guessed function
     
         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
    -    and make git-mergetool call this function instead of duplicating the
    -    logic. Also, let `$GIT_MERGETOOL_GUI` be set to determine whether or not
    -    the guitool will be selected.
    +    Write `get_merge_tool_guessed` to return whether or not the tool was
    +    guessed in addition to the actual tool and make git-mergetool call this
    +    function instead of duplicating the logic. Also, let
    +    `$GIT_MERGETOOL_GUI` be set to determine whether or not the guitool will
    +    be selected.
    +
    +    Make `get_merge_tool` use this function internally so that code
    +    duplication is reduced.
     
         Signed-off-by: Denton Liu <liu.denton@gmail.com>
     
    @@ -17,38 +21,32 @@
      --- a/Documentation/git-mergetool--lib.txt
      +++ b/Documentation/git-mergetool--lib.txt
     @@
    + 
      FUNCTIONS
      ---------
    - get_merge_tool::
    --	returns a merge tool.
    ++get_merge_tool_guessed::
     +	returns '$is_guessed:$merge_tool'. '$is_guessed' is 'true' if
     +	the tool was guessed, else 'false'. '$merge_tool' is the merge
     +	tool to use. '$GIT_MERGETOOL_GUI' may be set to 'true' to search
     +	for the appropriate guitool.
    ++
    + get_merge_tool::
    +-	returns a merge tool.
    ++	returns a merge tool. '$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
    - --- a/git-difftool--helper.sh
    - +++ b/git-difftool--helper.sh
    -@@
    - 	then
    - 		merge_tool="$GIT_DIFF_TOOL"
    - 	else
    --		merge_tool="$(get_merge_tool)" || exit
    -+		merge_tool="$(get_merge_tool | sed -e 's/^[a-z]*://')" || exit
    - 	fi
    - fi
    - 
    -
      diff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh
      --- a/git-mergetool--lib.sh
      +++ b/git-mergetool--lib.sh
     @@
    + 	echo "$merge_tool_path"
      }
      
    - get_merge_tool () {
    +-get_merge_tool () {
    ++get_merge_tool_guessed () {
     +	is_guessed=false
      	# Check if a merge tool has been configured
     -	merge_tool=$(get_configured_merge_tool)
    @@ -61,6 +59,10 @@
      	fi
     -	echo "$merge_tool"
     +	echo "$is_guessed:$merge_tool"
    ++}
    ++
    ++get_merge_tool () {
    ++	get_merge_tool_guessed | sed -e 's/^[a-z]*://'
      }
      
      mergetool_find_win32_cmd () {
    @@ -81,7 +83,7 @@
     -			guessed_merge_tool=true
     -		fi
     +		IFS=':' read guessed_merge_tool merge_tool <<-EOF
    -+		$(GIT_MERGETOOL_GUI=$gui_tool get_merge_tool)
    ++		$(GIT_MERGETOOL_GUI=$gui_tool get_merge_tool_guessed)
     +		EOF
      	fi
      	merge_keep_backup="$(git config --bool mergetool.keepBackup || echo true)"
3:  de1b897a11 = 3:  24db1afeee mergetool: fallback to tool when guitool unavailable
4:  a272594bd2 = 4:  6f65b5c913 difftool: make --gui, --tool and --extcmd mutually exclusive
5:  4fc3f84bad = 5:  5a24772219 difftool: fallback on merge.guitool
-- 
2.21.0.1000.g11cd861522


^ permalink raw reply	[flat|nested] 48+ messages in thread

* [PATCH v2 1/5] t7610: add mergetool --gui tests
  2019-04-23  8:53 ` [PATCH v2 0/5] difftool and mergetool improvements Denton Liu
@ 2019-04-23  8:53   ` Denton Liu
  2019-04-24  7:07     ` Junio C Hamano
  2019-04-23  8:54   ` [PATCH v2 2/5] mergetool: use get_merge_tool_guessed function Denton Liu
                     ` (4 subsequent siblings)
  5 siblings, 1 reply; 48+ messages in thread
From: Denton Liu @ 2019-04-23  8:53 UTC (permalink / raw)
  To: Git Mailing List
  Cc: Johannes Schindelin, David Aguilar, Jeff Hostetler, Eric Sunshine

In 063f2bdbf7 (mergetool: accept -g/--[no-]gui as arguments,
2018-10-24), mergetool was taught the --gui option but no tests were
added to ensure that it was working properly. Add a test to ensure that
it works.

Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
 t/t7610-mergetool.sh | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/t/t7610-mergetool.sh b/t/t7610-mergetool.sh
index a9fb971615..5f37d7a1ff 100755
--- a/t/t7610-mergetool.sh
+++ b/t/t7610-mergetool.sh
@@ -145,6 +145,28 @@ test_expect_success 'custom mergetool' '
 	git commit -m "branch1 resolved with mergetool"
 '
 
+test_expect_success 'gui mergetool' '
+	test_config merge.guitool myguitool &&
+	test_config mergetool.myguitool.cmd "(printf \"gui \" && cat \"\$REMOTE\") >\"\$MERGED\"" &&
+	test_config mergetool.myguitool.trustExitCode true &&
+	test_when_finished "git reset --hard" &&
+	git checkout -b test$test_count branch1 &&
+	git submodule update -N &&
+	test_must_fail git merge master >/dev/null 2>&1 &&
+	( yes "" | git mergetool --gui both >/dev/null 2>&1 ) &&
+	( yes "" | git mergetool -g file1 file1 ) &&
+	( yes "" | git mergetool --gui file2 "spaced name" >/dev/null 2>&1 ) &&
+	( yes "" | git mergetool --gui subdir/file3 >/dev/null 2>&1 ) &&
+	( yes "d" | git mergetool --gui file11 >/dev/null 2>&1 ) &&
+	( yes "d" | git mergetool --gui file12 >/dev/null 2>&1 ) &&
+	( yes "l" | git mergetool --gui submod >/dev/null 2>&1 ) &&
+	test "$(cat file1)" = "gui master updated" &&
+	test "$(cat file2)" = "gui master new" &&
+	test "$(cat subdir/file3)" = "gui master new sub" &&
+	test "$(cat submod/bar)" = "branch1 submodule" &&
+	git commit -m "branch1 resolved with mergetool"
+'
+
 test_expect_success 'mergetool crlf' '
 	test_when_finished "git reset --hard" &&
 	# This test_config line must go after the above reset line so that
-- 
2.21.0.1000.g11cd861522


^ permalink raw reply related	[flat|nested] 48+ messages in thread

* [PATCH v2 2/5] mergetool: use get_merge_tool_guessed function
  2019-04-23  8:53 ` [PATCH v2 0/5] difftool and mergetool improvements Denton Liu
  2019-04-23  8:53   ` [PATCH v2 1/5] t7610: add mergetool --gui tests Denton Liu
@ 2019-04-23  8:54   ` Denton Liu
  2019-04-24  7:27     ` Junio C Hamano
  2019-04-23  8:54   ` [PATCH v2 3/5] mergetool: fallback to tool when guitool unavailable Denton Liu
                     ` (3 subsequent siblings)
  5 siblings, 1 reply; 48+ messages in thread
From: Denton Liu @ 2019-04-23  8:54 UTC (permalink / raw)
  To: Git Mailing List
  Cc: Johannes Schindelin, David Aguilar, Jeff Hostetler, Eric Sunshine

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.

Write `get_merge_tool_guessed` to return whether or not the tool was
guessed in addition to the actual tool and make git-mergetool call this
function instead of duplicating the logic. Also, let
`$GIT_MERGETOOL_GUI` be set to determine whether or not the guitool will
be selected.

Make `get_merge_tool` use this function internally so that code
duplication is reduced.

Signed-off-by: Denton Liu <liu.denton@gmail.com>
---

After thinking about it for a while, I realised that if it was easy to
find one (albeit old) public project using our code, there should be
many others who we don't know about that will also be using our code.

Let's save them the trouble and just introduce a new function instead of
changing the behaviour of the old one.

---
 Documentation/git-mergetool--lib.txt |  9 ++++++++-
 git-mergetool--lib.sh                | 12 +++++++++---
 git-mergetool.sh                     | 11 +++--------
 3 files changed, 20 insertions(+), 12 deletions(-)

diff --git a/Documentation/git-mergetool--lib.txt b/Documentation/git-mergetool--lib.txt
index 055550b2bc..343268d885 100644
--- a/Documentation/git-mergetool--lib.txt
+++ b/Documentation/git-mergetool--lib.txt
@@ -27,8 +27,15 @@ to define the operation mode for the functions listed below.
 
 FUNCTIONS
 ---------
+get_merge_tool_guessed::
+	returns '$is_guessed:$merge_tool'. '$is_guessed' is 'true' if
+	the tool was guessed, else 'false'. '$merge_tool' is the merge
+	tool to use. '$GIT_MERGETOOL_GUI' may be set to 'true' to search
+	for the appropriate guitool.
+
 get_merge_tool::
-	returns a merge tool.
+	returns a merge tool. '$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-mergetool--lib.sh b/git-mergetool--lib.sh
index 83bf52494c..5eedb1a08a 100644
--- a/git-mergetool--lib.sh
+++ b/git-mergetool--lib.sh
@@ -402,15 +402,21 @@ get_merge_tool_path () {
 	echo "$merge_tool_path"
 }
 
-get_merge_tool () {
+get_merge_tool_guessed () {
+	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)
 	# 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
+		is_guessed=true
 	fi
-	echo "$merge_tool"
+	echo "$is_guessed:$merge_tool"
+}
+
+get_merge_tool () {
+	get_merge_tool_guessed | sed -e 's/^[a-z]*://'
 }
 
 mergetool_find_win32_cmd () {
diff --git a/git-mergetool.sh b/git-mergetool.sh
index 01b9ad59b2..63e4da1b2f 100755
--- a/git-mergetool.sh
+++ b/git-mergetool.sh
@@ -449,14 +449,9 @@ 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"
-		then
-			merge_tool=$(guess_merge_tool) || exit
-			guessed_merge_tool=true
-		fi
+		IFS=':' read guessed_merge_tool merge_tool <<-EOF
+		$(GIT_MERGETOOL_GUI=$gui_tool get_merge_tool_guessed)
+		EOF
 	fi
 	merge_keep_backup="$(git config --bool mergetool.keepBackup || echo true)"
 	merge_keep_temporaries="$(git config --bool mergetool.keepTemporaries || echo false)"
-- 
2.21.0.1000.g11cd861522


^ permalink raw reply related	[flat|nested] 48+ messages in thread

* [PATCH v2 3/5] mergetool: fallback to tool when guitool unavailable
  2019-04-23  8:53 ` [PATCH v2 0/5] difftool and mergetool improvements Denton Liu
  2019-04-23  8:53   ` [PATCH v2 1/5] t7610: add mergetool --gui tests Denton Liu
  2019-04-23  8:54   ` [PATCH v2 2/5] mergetool: use get_merge_tool_guessed function Denton Liu
@ 2019-04-23  8:54   ` Denton Liu
  2019-04-23  8:54   ` [PATCH v2 4/5] difftool: make --gui, --tool and --extcmd mutually exclusive Denton Liu
                     ` (2 subsequent siblings)
  5 siblings, 0 replies; 48+ messages in thread
From: Denton Liu @ 2019-04-23  8:54 UTC (permalink / raw)
  To: Git Mailing List
  Cc: Johannes Schindelin, David Aguilar, Jeff Hostetler, Eric Sunshine

In git-difftool, if the tool is called with --gui but `diff.guitool` is
not set, it falls back to `diff.tool`. Make git-mergetool also fallback
from `merge.guitool` to `merge.tool` if the former is undefined.

If git-difftool were to use `get_configured_mergetool`, it would also
get the fallback behaviour in the following precedence:

1. diff.guitool
2. merge.guitool
3. diff.tool
4. merge.tool

Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
 Documentation/git-mergetool.txt |  4 +++-
 git-mergetool--lib.sh           | 27 ++++++++++++++++++---------
 t/t7610-mergetool.sh            | 19 +++++++++++++++++++
 3 files changed, 40 insertions(+), 10 deletions(-)

diff --git a/Documentation/git-mergetool.txt b/Documentation/git-mergetool.txt
index 0c7975a050..6b14702e78 100644
--- a/Documentation/git-mergetool.txt
+++ b/Documentation/git-mergetool.txt
@@ -83,7 +83,9 @@ success of the resolution after the custom tool has exited.
 --gui::
 	When 'git-mergetool' is invoked with the `-g` or `--gui` option
 	the default merge tool will be read from the configured
-	`merge.guitool` variable instead of `merge.tool`.
+	`merge.guitool` variable instead of `merge.tool`. If
+	`merge.guitool` is not set, we will fallback to the tool
+	configured under `merge.tool`.
 
 --no-gui::
 	This overrides a previous `-g` or `--gui` setting and reads the
diff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh
index 5eedb1a08a..6f477ae77d 100644
--- a/git-mergetool--lib.sh
+++ b/git-mergetool--lib.sh
@@ -350,20 +350,29 @@ guess_merge_tool () {
 }
 
 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
-		gui_prefix=gui
+		sections="diff $sections"
 	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
 	then
-		merge_tool=$(git config diff.${gui_prefix}tool || git config merge.${gui_prefix}tool)
-	else
-		merge_tool=$(git config merge.${gui_prefix}tool)
+		keys="guitool $keys"
 	fi
+
+	IFS=' '
+	for key in $keys
+	do
+		for section in $sections
+		do
+			merge_tool=$(git config $section.$key) && break 2
+		done
+	done
+
 	if test -n "$merge_tool" && ! valid_tool "$merge_tool"
 	then
 		echo >&2 "git config option $TOOL_MODE.${gui_prefix}tool set to unknown tool: $merge_tool"
diff --git a/t/t7610-mergetool.sh b/t/t7610-mergetool.sh
index 5f37d7a1ff..bc2c9eaa30 100755
--- a/t/t7610-mergetool.sh
+++ b/t/t7610-mergetool.sh
@@ -167,6 +167,25 @@ test_expect_success 'gui mergetool' '
 	git commit -m "branch1 resolved with mergetool"
 '
 
+test_expect_success 'gui mergetool without merge.guitool set fallsback to merge.tool' '
+	test_when_finished "git reset --hard" &&
+	git checkout -b test$test_count branch1 &&
+	git submodule update -N &&
+	test_must_fail git merge master >/dev/null 2>&1 &&
+	( yes "" | git mergetool --gui both >/dev/null 2>&1 ) &&
+	( yes "" | git mergetool -g file1 file1 ) &&
+	( yes "" | git mergetool --gui file2 "spaced name" >/dev/null 2>&1 ) &&
+	( yes "" | git mergetool --gui subdir/file3 >/dev/null 2>&1 ) &&
+	( yes "d" | git mergetool --gui file11 >/dev/null 2>&1 ) &&
+	( yes "d" | git mergetool --gui file12 >/dev/null 2>&1 ) &&
+	( yes "l" | git mergetool --gui submod >/dev/null 2>&1 ) &&
+	test "$(cat file1)" = "master updated" &&
+	test "$(cat file2)" = "master new" &&
+	test "$(cat subdir/file3)" = "master new sub" &&
+	test "$(cat submod/bar)" = "branch1 submodule" &&
+	git commit -m "branch1 resolved with mergetool"
+'
+
 test_expect_success 'mergetool crlf' '
 	test_when_finished "git reset --hard" &&
 	# This test_config line must go after the above reset line so that
-- 
2.21.0.1000.g11cd861522


^ permalink raw reply related	[flat|nested] 48+ messages in thread

* [PATCH v2 4/5] difftool: make --gui, --tool and --extcmd mutually exclusive
  2019-04-23  8:53 ` [PATCH v2 0/5] difftool and mergetool improvements Denton Liu
                     ` (2 preceding siblings ...)
  2019-04-23  8:54   ` [PATCH v2 3/5] mergetool: fallback to tool when guitool unavailable Denton Liu
@ 2019-04-23  8:54   ` Denton Liu
  2019-04-23  8:54   ` [PATCH v2 5/5] difftool: fallback on merge.guitool Denton Liu
  2019-04-24 22:46   ` [PATCH v3 0/6] difftool and mergetool improvements Denton Liu
  5 siblings, 0 replies; 48+ messages in thread
From: Denton Liu @ 2019-04-23  8:54 UTC (permalink / raw)
  To: Git Mailing List
  Cc: Johannes Schindelin, David Aguilar, Jeff Hostetler, Eric Sunshine

In git-difftool, these options specify which tool to ultimately run. As
a result, they are logically conflicting. Explicitly disallow these
options from being used together.

Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
 builtin/difftool.c  | 3 +++
 t/t7800-difftool.sh | 8 ++++++++
 2 files changed, 11 insertions(+)

diff --git a/builtin/difftool.c b/builtin/difftool.c
index a3ea60ea71..65bba90338 100644
--- a/builtin/difftool.c
+++ b/builtin/difftool.c
@@ -731,6 +731,9 @@ int cmd_difftool(int argc, const char **argv, const char *prefix)
 	setenv(GIT_DIR_ENVIRONMENT, absolute_path(get_git_dir()), 1);
 	setenv(GIT_WORK_TREE_ENVIRONMENT, absolute_path(get_git_work_tree()), 1);
 
+	if (use_gui_tool + !!difftool_cmd + !!extcmd > 1)
+		die(_("--gui, --tool and --extcmd are mutually exclusive"));
+
 	if (use_gui_tool && diff_gui_tool && *diff_gui_tool)
 		setenv("GIT_DIFF_TOOL", diff_gui_tool, 1);
 	else if (difftool_cmd) {
diff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh
index bb9a7f4ff9..107f31213d 100755
--- a/t/t7800-difftool.sh
+++ b/t/t7800-difftool.sh
@@ -705,4 +705,12 @@ test_expect_success SYMLINKS 'difftool --dir-diff handles modified symlinks' '
 	test_cmp expect actual
 '
 
+test_expect_success 'difftool --gui, --tool and --extcmd are exclusive' '
+	difftool_test_setup &&
+	test_must_fail git difftool --gui --tool=test-tool &&
+	test_must_fail git difftool --gui --extcmd=cat &&
+	test_must_fail git difftool --tool=test-tool --extcmd=cat &&
+	test_must_fail git difftool --gui --tool=test-tool --extcmd=cat
+'
+
 test_done
-- 
2.21.0.1000.g11cd861522


^ permalink raw reply related	[flat|nested] 48+ messages in thread

* [PATCH v2 5/5] difftool: fallback on merge.guitool
  2019-04-23  8:53 ` [PATCH v2 0/5] difftool and mergetool improvements Denton Liu
                     ` (3 preceding siblings ...)
  2019-04-23  8:54   ` [PATCH v2 4/5] difftool: make --gui, --tool and --extcmd mutually exclusive Denton Liu
@ 2019-04-23  8:54   ` Denton Liu
  2019-04-24 22:46   ` [PATCH v3 0/6] difftool and mergetool improvements Denton Liu
  5 siblings, 0 replies; 48+ messages in thread
From: Denton Liu @ 2019-04-23  8:54 UTC (permalink / raw)
  To: Git Mailing List
  Cc: Johannes Schindelin, David Aguilar, Jeff Hostetler, Eric Sunshine

In git-difftool.txt, it says

	'git difftool' falls back to 'git mergetool' config variables when the
	difftool equivalents have not been defined.

However, when `diff.guitool` is missing, it doesn't fallback to
anything. Make git-difftool fallback to `merge.guitool` when `diff.guitool` is
missing.

Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
 Documentation/git-difftool.txt |  4 +++-
 builtin/difftool.c             | 10 ++--------
 t/t7800-difftool.sh            | 16 ++++++++++++++++
 3 files changed, 21 insertions(+), 9 deletions(-)

diff --git a/Documentation/git-difftool.txt b/Documentation/git-difftool.txt
index 96c26e6aa8..484c485fd0 100644
--- a/Documentation/git-difftool.txt
+++ b/Documentation/git-difftool.txt
@@ -90,7 +90,9 @@ instead.  `--no-symlinks` is the default on Windows.
 	When 'git-difftool' is invoked with the `-g` or `--gui` option
 	the default diff tool will be read from the configured
 	`diff.guitool` variable instead of `diff.tool`. The `--no-gui`
-	option can be used to override this setting.
+	option can be used to override this setting. If `diff.guitool`
+	is not set, we will fallback in the order of `merge.guitool`,
+	`diff.tool`, `merge.tool` until a tool is found.
 
 --[no-]trust-exit-code::
 	'git-difftool' invokes a diff tool individually on each file.
diff --git a/builtin/difftool.c b/builtin/difftool.c
index 65bba90338..10660639c0 100644
--- a/builtin/difftool.c
+++ b/builtin/difftool.c
@@ -24,7 +24,6 @@
 #include "object-store.h"
 #include "dir.h"
 
-static char *diff_gui_tool;
 static int trust_exit_code;
 
 static const char *const builtin_difftool_usage[] = {
@@ -34,11 +33,6 @@ static const char *const builtin_difftool_usage[] = {
 
 static int difftool_config(const char *var, const char *value, void *cb)
 {
-	if (!strcmp(var, "diff.guitool")) {
-		diff_gui_tool = xstrdup(value);
-		return 0;
-	}
-
 	if (!strcmp(var, "difftool.trustexitcode")) {
 		trust_exit_code = git_config_bool(var, value);
 		return 0;
@@ -734,8 +728,8 @@ int cmd_difftool(int argc, const char **argv, const char *prefix)
 	if (use_gui_tool + !!difftool_cmd + !!extcmd > 1)
 		die(_("--gui, --tool and --extcmd are mutually exclusive"));
 
-	if (use_gui_tool && diff_gui_tool && *diff_gui_tool)
-		setenv("GIT_DIFF_TOOL", diff_gui_tool, 1);
+	if (use_gui_tool)
+		setenv("GIT_MERGETOOL_GUI", "true", 1);
 	else if (difftool_cmd) {
 		if (*difftool_cmd)
 			setenv("GIT_DIFF_TOOL", difftool_cmd, 1);
diff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh
index 107f31213d..ae90701a12 100755
--- a/t/t7800-difftool.sh
+++ b/t/t7800-difftool.sh
@@ -279,11 +279,27 @@ test_expect_success 'difftool + mergetool config variables' '
 	echo branch >expect &&
 	git difftool --no-prompt branch >actual &&
 	test_cmp expect actual &&
+	git difftool --gui --no-prompt branch >actual &&
+	test_cmp expect actual &&
 
 	# set merge.tool to something bogus, diff.tool to test-tool
 	test_config merge.tool bogus-tool &&
 	test_config diff.tool test-tool &&
 	git difftool --no-prompt branch >actual &&
+	test_cmp expect actual &&
+	git difftool --gui --no-prompt branch >actual &&
+	test_cmp expect actual &&
+
+	# set merge.tool, diff.tool to something bogus, merge.guitool to test-tool
+	test_config diff.tool bogus-tool &&
+	test_config merge.guitool test-tool &&
+	git difftool --gui --no-prompt branch >actual &&
+	test_cmp expect actual &&
+
+	# set merge.tool, diff.tool, merge.guitool to something bogus, diff.guitool to test-tool
+	test_config merge.guitool bogus-tool &&
+	test_config diff.guitool test-tool &&
+	git difftool --gui --no-prompt branch >actual &&
 	test_cmp expect actual
 '
 
-- 
2.21.0.1000.g11cd861522


^ permalink raw reply related	[flat|nested] 48+ messages in thread

* Re: [PATCH v2 1/5] t7610: add mergetool --gui tests
  2019-04-23  8:53   ` [PATCH v2 1/5] t7610: add mergetool --gui tests Denton Liu
@ 2019-04-24  7:07     ` Junio C Hamano
  0 siblings, 0 replies; 48+ messages in thread
From: Junio C Hamano @ 2019-04-24  7:07 UTC (permalink / raw)
  To: Denton Liu
  Cc: Git Mailing List, Johannes Schindelin, David Aguilar,
	Jeff Hostetler, Eric Sunshine

Denton Liu <liu.denton@gmail.com> writes:

> In 063f2bdbf7 (mergetool: accept -g/--[no-]gui as arguments,
> 2018-10-24), mergetool was taught the --gui option but no tests were
> added to ensure that it was working properly. Add a test to ensure that
> it works.
>
> Signed-off-by: Denton Liu <liu.denton@gmail.com>
> ---
>  t/t7610-mergetool.sh | 22 ++++++++++++++++++++++
>  1 file changed, 22 insertions(+)
>
> diff --git a/t/t7610-mergetool.sh b/t/t7610-mergetool.sh
> index a9fb971615..5f37d7a1ff 100755
> --- a/t/t7610-mergetool.sh
> +++ b/t/t7610-mergetool.sh
> @@ -145,6 +145,28 @@ test_expect_success 'custom mergetool' '
>  	git commit -m "branch1 resolved with mergetool"
>  '
>  
> +test_expect_success 'gui mergetool' '
> +	test_config merge.guitool myguitool &&
> +	test_config mergetool.myguitool.cmd "(printf \"gui \" && cat \"\$REMOTE\") >\"\$MERGED\"" &&
> +	test_config mergetool.myguitool.trustExitCode true &&
> +	test_when_finished "git reset --hard" &&
> +	git checkout -b test$test_count branch1 &&
> +	git submodule update -N &&

> +	test_must_fail git merge master >/dev/null 2>&1 &&
> +	( yes "" | git mergetool --gui both >/dev/null 2>&1 ) &&
> +	( yes "" | git mergetool -g file1 file1 ) &&
> +	( yes "" | git mergetool --gui file2 "spaced name" >/dev/null 2>&1 ) &&
> +	( yes "" | git mergetool --gui subdir/file3 >/dev/null 2>&1 ) &&
> +	( yes "d" | git mergetool --gui file11 >/dev/null 2>&1 ) &&
> +	( yes "d" | git mergetool --gui file12 >/dev/null 2>&1 ) &&
> +	( yes "l" | git mergetool --gui submod >/dev/null 2>&1 ) &&

We usually discourage suppressing the output from git commands being
tested like the above via redirection.  This new testlet seems to
mimick the way some of the existing ones are written, but it seems
that not all invocations of mergetool in this file discard the
output.  Is there a particular reason why there are two styles?
If not, I think we would want to standardize on *not* discarding.

Thanks.

^ permalink raw reply	[flat|nested] 48+ messages in thread

* Re: [PATCH v2 2/5] mergetool: use get_merge_tool_guessed function
  2019-04-23  8:54   ` [PATCH v2 2/5] mergetool: use get_merge_tool_guessed function Denton Liu
@ 2019-04-24  7:27     ` Junio C Hamano
  0 siblings, 0 replies; 48+ messages in thread
From: Junio C Hamano @ 2019-04-24  7:27 UTC (permalink / raw)
  To: Denton Liu
  Cc: Git Mailing List, Johannes Schindelin, David Aguilar,
	Jeff Hostetler, Eric Sunshine

Denton Liu <liu.denton@gmail.com> writes:

> +get_merge_tool_guessed () {
> +	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)
>  	# 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
> +		is_guessed=true
>  	fi
> -	echo "$merge_tool"
> +	echo "$is_guessed:$merge_tool"
> +}
> +
> +get_merge_tool () {
> +	get_merge_tool_guessed | sed -e 's/^[a-z]*://'
>  }

Yuck.  Returning a:b is fine if the main use is to match that string
using shell builtins like "test" and "case", but piping to "sed"
feels a bit too much overhead.  Especially given that the other
reader in git-emrgetool.sh is not protected for $merge_tool that has
a colon in it.  Do not try to be too cute and end up with a hack
that is both inefficient and brittle at the same time.

Possible alternatives:

 - Because variables in bourne family of shells are global, the
   caller can easily peek at $is_guessed; or

 - One bit "did we guess, or did we get from the user?" boolean
   choice can sufficiently be conveyed by ending the fuction like so
   instead:

		...
	fi
		echo "$merge_tool"
		test "$is_guessed" = true
	}

> diff --git a/git-mergetool.sh b/git-mergetool.sh
> index 01b9ad59b2..63e4da1b2f 100755
> --- a/git-mergetool.sh
> +++ b/git-mergetool.sh
> @@ -449,14 +449,9 @@ 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"
> -		then
> -			merge_tool=$(guess_merge_tool) || exit
> -			guessed_merge_tool=true
> -		fi
> +		IFS=':' read guessed_merge_tool merge_tool <<-EOF
> +		$(GIT_MERGETOOL_GUI=$gui_tool get_merge_tool_guessed)
> +		EOF

With the "let the return code speak" alternative, this would become
something like

	if merge_tool=$(GIT_MERGETOOL_GUI=$gui_tool; get_merge_tool_guessed)
	then
		guessed_merge_tool=true
	else
		guessed_merge_tool=false
	fi
	
I do not know what you are trying with GIT_MERGETOOL_GUI=$gui_tool
before the shell function, though.  It does not work as one-shot
assignment to an environment variable.  I _think_ it is to feed the
all-caps variable to get_configured_merge_tool that is invoked by
the get_merge_tool_guessed function, so it does not have to be
exported as an environment in the first place, so in the above
illustration, I simply wrote an assignment statement, followed by a
separate statement that is a parameterless call of a shell function,
separated by a semicolon.

>  	fi
>  	merge_keep_backup="$(git config --bool mergetool.keepBackup || echo true)"
>  	merge_keep_temporaries="$(git config --bool mergetool.keepTemporaries || echo false)"

^ permalink raw reply	[flat|nested] 48+ messages in thread

* [PATCH v3 0/6] difftool and mergetool improvements
  2019-04-23  8:53 ` [PATCH v2 0/5] difftool and mergetool improvements Denton Liu
                     ` (4 preceding siblings ...)
  2019-04-23  8:54   ` [PATCH v2 5/5] difftool: fallback on merge.guitool Denton Liu
@ 2019-04-24 22:46   ` Denton Liu
  2019-04-24 22:46     ` [PATCH v3 1/6] t7610: unsuppress output Denton Liu
                       ` (6 more replies)
  5 siblings, 7 replies; 48+ messages in thread
From: Denton Liu @ 2019-04-24 22:46 UTC (permalink / raw)
  To: Git Mailing List
  Cc: Johannes Schindelin, David Aguilar, Jeff Hostetler,
	Eric Sunshine, Junio C Hamano

Thanks for the review, Junio.

I hadn't thought about using the return code for `get_merge_tool` so
thanks for that suggestion. It should be a lot cleaner now.

---

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 (6):
  t7610: unsuppress output
  t7610: add mergetool --gui tests
  mergetool: use get_merge_tool 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                |  32 ++++--
 git-mergetool.sh                     |  12 +-
 t/t7610-mergetool.sh                 | 163 +++++++++++++++++----------
 t/t7800-difftool.sh                  |  24 ++++
 9 files changed, 167 insertions(+), 91 deletions(-)

Range-diff against v2:
-:  ---------- > 1:  9f9922cab3 t7610: unsuppress output
1:  c436765684 ! 2:  0f632ca6bf t7610: add mergetool --gui tests
    @@ -23,14 +23,14 @@
     +	test_when_finished "git reset --hard" &&
     +	git checkout -b test$test_count branch1 &&
     +	git submodule update -N &&
    -+	test_must_fail git merge master >/dev/null 2>&1 &&
    -+	( yes "" | git mergetool --gui both >/dev/null 2>&1 ) &&
    ++	test_must_fail git merge master &&
    ++	( yes "" | git mergetool --gui both ) &&
     +	( yes "" | git mergetool -g file1 file1 ) &&
    -+	( yes "" | git mergetool --gui file2 "spaced name" >/dev/null 2>&1 ) &&
    -+	( yes "" | git mergetool --gui subdir/file3 >/dev/null 2>&1 ) &&
    -+	( yes "d" | git mergetool --gui file11 >/dev/null 2>&1 ) &&
    -+	( yes "d" | git mergetool --gui file12 >/dev/null 2>&1 ) &&
    -+	( yes "l" | git mergetool --gui submod >/dev/null 2>&1 ) &&
    ++	( yes "" | git mergetool --gui file2 "spaced name" ) &&
    ++	( yes "" | git mergetool --gui subdir/file3 ) &&
    ++	( yes "d" | git mergetool --gui file11 ) &&
    ++	( yes "d" | git mergetool --gui file12 ) &&
    ++	( yes "l" | git mergetool --gui submod ) &&
     +	test "$(cat file1)" = "gui master updated" &&
     +	test "$(cat file2)" = "gui master new" &&
     +	test "$(cat subdir/file3)" = "gui master new sub" &&
2:  9f8e76a421 < -:  ---------- mergetool: use get_merge_tool_guessed function
-:  ---------- > 3:  ff83d25ff2 mergetool: use get_merge_tool function
3:  4847e64e46 ! 4:  e975fe4a8b mergetool: fallback to tool when guitool unavailable
    @@ -81,18 +81,18 @@
      	git commit -m "branch1 resolved with mergetool"
      '
      
    -+test_expect_success 'gui mergetool without merge.guitool set fallsback to merge.tool' '
    ++test_expect_success 'gui mergetool without merge.guitool set falls back to merge.tool' '
     +	test_when_finished "git reset --hard" &&
     +	git checkout -b test$test_count branch1 &&
     +	git submodule update -N &&
    -+	test_must_fail git merge master >/dev/null 2>&1 &&
    -+	( yes "" | git mergetool --gui both >/dev/null 2>&1 ) &&
    ++	test_must_fail git merge master &&
    ++	( yes "" | git mergetool --gui both ) &&
     +	( yes "" | git mergetool -g file1 file1 ) &&
    -+	( yes "" | git mergetool --gui file2 "spaced name" >/dev/null 2>&1 ) &&
    -+	( yes "" | git mergetool --gui subdir/file3 >/dev/null 2>&1 ) &&
    -+	( yes "d" | git mergetool --gui file11 >/dev/null 2>&1 ) &&
    -+	( yes "d" | git mergetool --gui file12 >/dev/null 2>&1 ) &&
    -+	( yes "l" | git mergetool --gui submod >/dev/null 2>&1 ) &&
    ++	( yes "" | git mergetool --gui file2 "spaced name" ) &&
    ++	( yes "" | git mergetool --gui subdir/file3 ) &&
    ++	( yes "d" | git mergetool --gui file11 ) &&
    ++	( yes "d" | git mergetool --gui file12 ) &&
    ++	( yes "l" | git mergetool --gui submod ) &&
     +	test "$(cat file1)" = "master updated" &&
     +	test "$(cat file2)" = "master new" &&
     +	test "$(cat subdir/file3)" = "master new sub" &&
4:  bc8cadaf55 = 5:  bc3e229171 difftool: make --gui, --tool and --extcmd mutually exclusive
5:  e5e4dc3dd2 = 6:  f39b15efbd difftool: fallback on merge.guitool
-- 
2.21.0.1000.g7817e26e80


^ permalink raw reply	[flat|nested] 48+ messages in thread

* [PATCH v3 1/6] t7610: unsuppress output
  2019-04-24 22:46   ` [PATCH v3 0/6] difftool and mergetool improvements Denton Liu
@ 2019-04-24 22:46     ` Denton Liu
  2019-04-25  2:31       ` Junio C Hamano
  2019-04-24 22:46     ` [PATCH v3 2/6] t7610: add mergetool --gui tests Denton Liu
                       ` (5 subsequent siblings)
  6 siblings, 1 reply; 48+ messages in thread
From: Denton Liu @ 2019-04-24 22:46 UTC (permalink / raw)
  To: Git Mailing List
  Cc: Johannes Schindelin, David Aguilar, Jeff Hostetler,
	Eric Sunshine, Junio C Hamano

The output for commands used to be suppressed by redirecting both stdout
and stderr to /dev/null. However, this should not happen since the
output is useful for debugging and, without the "-v" flag, test scripts
don't output anyway.

Unsuppress the output by removing the redirections to /dev/null.

Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
 t/t7610-mergetool.sh | 122 +++++++++++++++++++++----------------------
 1 file changed, 61 insertions(+), 61 deletions(-)

diff --git a/t/t7610-mergetool.sh b/t/t7610-mergetool.sh
index a9fb971615..69711487dd 100755
--- a/t/t7610-mergetool.sh
+++ b/t/t7610-mergetool.sh
@@ -130,14 +130,14 @@ test_expect_success 'custom mergetool' '
 	test_when_finished "git reset --hard" &&
 	git checkout -b test$test_count branch1 &&
 	git submodule update -N &&
-	test_must_fail git merge master >/dev/null 2>&1 &&
-	( yes "" | git mergetool both >/dev/null 2>&1 ) &&
+	test_must_fail git merge master &&
+	( yes "" | git mergetool both ) &&
 	( yes "" | git mergetool file1 file1 ) &&
-	( yes "" | git mergetool file2 "spaced name" >/dev/null 2>&1 ) &&
-	( yes "" | git mergetool subdir/file3 >/dev/null 2>&1 ) &&
-	( yes "d" | git mergetool file11 >/dev/null 2>&1 ) &&
-	( yes "d" | git mergetool file12 >/dev/null 2>&1 ) &&
-	( yes "l" | git mergetool submod >/dev/null 2>&1 ) &&
+	( yes "" | git mergetool file2 "spaced name" ) &&
+	( yes "" | git mergetool subdir/file3 ) &&
+	( yes "d" | git mergetool file11 ) &&
+	( yes "d" | git mergetool file12 ) &&
+	( yes "l" | git mergetool submod ) &&
 	test "$(cat file1)" = "master updated" &&
 	test "$(cat file2)" = "master new" &&
 	test "$(cat subdir/file3)" = "master new sub" &&
@@ -153,15 +153,15 @@ test_expect_success 'mergetool crlf' '
 	# test_when_finished is LIFO.)
 	test_config core.autocrlf true &&
 	git checkout -b test$test_count branch1 &&
-	test_must_fail git merge master >/dev/null 2>&1 &&
-	( yes "" | git mergetool file1 >/dev/null 2>&1 ) &&
-	( yes "" | git mergetool file2 >/dev/null 2>&1 ) &&
-	( yes "" | git mergetool "spaced name" >/dev/null 2>&1 ) &&
-	( yes "" | git mergetool both >/dev/null 2>&1 ) &&
-	( yes "" | git mergetool subdir/file3 >/dev/null 2>&1 ) &&
-	( yes "d" | git mergetool file11 >/dev/null 2>&1 ) &&
-	( yes "d" | git mergetool file12 >/dev/null 2>&1 ) &&
-	( yes "r" | git mergetool submod >/dev/null 2>&1 ) &&
+	test_must_fail git merge master &&
+	( yes "" | git mergetool file1 ) &&
+	( yes "" | git mergetool file2 ) &&
+	( yes "" | git mergetool "spaced name" ) &&
+	( yes "" | git mergetool both ) &&
+	( yes "" | git mergetool subdir/file3 ) &&
+	( yes "d" | git mergetool file11 ) &&
+	( yes "d" | git mergetool file12 ) &&
+	( yes "r" | git mergetool submod ) &&
 	test "$(printf x | cat file1 -)" = "$(printf "master updated\r\nx")" &&
 	test "$(printf x | cat file2 -)" = "$(printf "master new\r\nx")" &&
 	test "$(printf x | cat subdir/file3 -)" = "$(printf "master new sub\r\nx")" &&
@@ -176,8 +176,8 @@ test_expect_success 'mergetool in subdir' '
 	git submodule update -N &&
 	(
 		cd subdir &&
-		test_must_fail git merge master >/dev/null 2>&1 &&
-		( yes "" | git mergetool file3 >/dev/null 2>&1 ) &&
+		test_must_fail git merge master &&
+		( yes "" | git mergetool file3 ) &&
 		test "$(cat file3)" = "master new sub"
 	)
 '
@@ -188,14 +188,14 @@ test_expect_success 'mergetool on file in parent dir' '
 	git submodule update -N &&
 	(
 		cd subdir &&
-		test_must_fail git merge master >/dev/null 2>&1 &&
-		( yes "" | git mergetool file3 >/dev/null 2>&1 ) &&
-		( yes "" | git mergetool ../file1 >/dev/null 2>&1 ) &&
-		( yes "" | git mergetool ../file2 ../spaced\ name >/dev/null 2>&1 ) &&
-		( yes "" | git mergetool ../both >/dev/null 2>&1 ) &&
-		( yes "d" | git mergetool ../file11 >/dev/null 2>&1 ) &&
-		( yes "d" | git mergetool ../file12 >/dev/null 2>&1 ) &&
-		( yes "l" | git mergetool ../submod >/dev/null 2>&1 ) &&
+		test_must_fail git merge master &&
+		( yes "" | git mergetool file3 ) &&
+		( yes "" | git mergetool ../file1 ) &&
+		( yes "" | git mergetool ../file2 ../spaced\ name ) &&
+		( yes "" | git mergetool ../both ) &&
+		( yes "d" | git mergetool ../file11 ) &&
+		( yes "d" | git mergetool ../file12 ) &&
+		( yes "l" | git mergetool ../submod ) &&
 		test "$(cat ../file1)" = "master updated" &&
 		test "$(cat ../file2)" = "master new" &&
 		test "$(cat ../submod/bar)" = "branch1 submodule" &&
@@ -209,9 +209,9 @@ test_expect_success 'mergetool skips autoresolved' '
 	git submodule update -N &&
 	test_must_fail git merge master &&
 	test -n "$(git ls-files -u)" &&
-	( yes "d" | git mergetool file11 >/dev/null 2>&1 ) &&
-	( yes "d" | git mergetool file12 >/dev/null 2>&1 ) &&
-	( yes "l" | git mergetool submod >/dev/null 2>&1 ) &&
+	( yes "d" | git mergetool file11 ) &&
+	( yes "d" | git mergetool file12 ) &&
+	( yes "l" | git mergetool submod ) &&
 	output="$(git mergetool --no-prompt)" &&
 	test "$output" = "No files need merging"
 '
@@ -259,9 +259,9 @@ test_expect_success 'mergetool skips resolved paths when rerere is active' '
 	rm -rf .git/rr-cache &&
 	git checkout -b test$test_count branch1 &&
 	git submodule update -N &&
-	test_must_fail git merge master >/dev/null 2>&1 &&
-	( yes "l" | git mergetool --no-prompt submod >/dev/null 2>&1 ) &&
-	( yes "d" "d" | git mergetool --no-prompt >/dev/null 2>&1 ) &&
+	test_must_fail git merge master &&
+	( yes "l" | git mergetool --no-prompt submod ) &&
+	( yes "d" "d" | git mergetool --no-prompt ) &&
 	git submodule update -N &&
 	output="$(yes "n" | git mergetool --no-prompt)" &&
 	test "$output" = "No files need merging"
@@ -369,9 +369,9 @@ test_expect_success 'deleted vs modified submodule' '
 	git checkout -b test$test_count.a test$test_count &&
 	test_must_fail git merge master &&
 	test -n "$(git ls-files -u)" &&
-	( yes "" | git mergetool file1 file2 spaced\ name subdir/file3 >/dev/null 2>&1 ) &&
-	( yes "" | git mergetool both >/dev/null 2>&1 ) &&
-	( yes "d" | git mergetool file11 file12 >/dev/null 2>&1 ) &&
+	( yes "" | git mergetool file1 file2 spaced\ name subdir/file3 ) &&
+	( yes "" | git mergetool both ) &&
+	( yes "d" | git mergetool file11 file12 ) &&
 	( yes "r" | git mergetool submod ) &&
 	rmdir submod && mv submod-movedaside submod &&
 	test "$(cat submod/bar)" = "branch1 submodule" &&
@@ -386,9 +386,9 @@ test_expect_success 'deleted vs modified submodule' '
 	git submodule update -N &&
 	test_must_fail git merge master &&
 	test -n "$(git ls-files -u)" &&
-	( yes "" | git mergetool file1 file2 spaced\ name subdir/file3 >/dev/null 2>&1 ) &&
-	( yes "" | git mergetool both >/dev/null 2>&1 ) &&
-	( yes "d" | git mergetool file11 file12 >/dev/null 2>&1 ) &&
+	( yes "" | git mergetool file1 file2 spaced\ name subdir/file3 ) &&
+	( yes "" | git mergetool both ) &&
+	( yes "d" | git mergetool file11 file12 ) &&
 	( yes "l" | git mergetool submod ) &&
 	test ! -e submod &&
 	output="$(git mergetool --no-prompt)" &&
@@ -400,9 +400,9 @@ test_expect_success 'deleted vs modified submodule' '
 	git submodule update -N &&
 	test_must_fail git merge test$test_count &&
 	test -n "$(git ls-files -u)" &&
-	( yes "" | git mergetool file1 file2 spaced\ name subdir/file3 >/dev/null 2>&1 ) &&
-	( yes "" | git mergetool both >/dev/null 2>&1 ) &&
-	( yes "d" | git mergetool file11 file12 >/dev/null 2>&1 ) &&
+	( yes "" | git mergetool file1 file2 spaced\ name subdir/file3 ) &&
+	( yes "" | git mergetool both ) &&
+	( yes "d" | git mergetool file11 file12 ) &&
 	( yes "r" | git mergetool submod ) &&
 	test ! -e submod &&
 	test -d submod.orig &&
@@ -416,9 +416,9 @@ test_expect_success 'deleted vs modified submodule' '
 	git submodule update -N &&
 	test_must_fail git merge test$test_count &&
 	test -n "$(git ls-files -u)" &&
-	( yes "" | git mergetool file1 file2 spaced\ name subdir/file3 >/dev/null 2>&1 ) &&
-	( yes "" | git mergetool both >/dev/null 2>&1 ) &&
-	( yes "d" | git mergetool file11 file12 >/dev/null 2>&1 ) &&
+	( yes "" | git mergetool file1 file2 spaced\ name subdir/file3 ) &&
+	( yes "" | git mergetool both ) &&
+	( yes "d" | git mergetool file11 file12 ) &&
 	( yes "l" | git mergetool submod ) &&
 	test "$(cat submod/bar)" = "master submodule" &&
 	git submodule update -N &&
@@ -440,9 +440,9 @@ test_expect_success 'file vs modified submodule' '
 	git checkout -b test$test_count.a branch1 &&
 	test_must_fail git merge master &&
 	test -n "$(git ls-files -u)" &&
-	( yes "" | git mergetool file1 file2 spaced\ name subdir/file3 >/dev/null 2>&1 ) &&
-	( yes "" | git mergetool both >/dev/null 2>&1 ) &&
-	( yes "d" | git mergetool file11 file12 >/dev/null 2>&1 ) &&
+	( yes "" | git mergetool file1 file2 spaced\ name subdir/file3 ) &&
+	( yes "" | git mergetool both ) &&
+	( yes "d" | git mergetool file11 file12 ) &&
 	( yes "r" | git mergetool submod ) &&
 	rmdir submod && mv submod-movedaside submod &&
 	test "$(cat submod/bar)" = "branch1 submodule" &&
@@ -456,9 +456,9 @@ test_expect_success 'file vs modified submodule' '
 	git checkout -b test$test_count.b test$test_count &&
 	test_must_fail git merge master &&
 	test -n "$(git ls-files -u)" &&
-	( yes "" | git mergetool file1 file2 spaced\ name subdir/file3 >/dev/null 2>&1 ) &&
-	( yes "" | git mergetool both >/dev/null 2>&1 ) &&
-	( yes "d" | git mergetool file11 file12 >/dev/null 2>&1 ) &&
+	( yes "" | git mergetool file1 file2 spaced\ name subdir/file3 ) &&
+	( yes "" | git mergetool both ) &&
+	( yes "d" | git mergetool file11 file12 ) &&
 	( yes "l" | git mergetool submod ) &&
 	git submodule update -N &&
 	test "$(cat submod)" = "not a submodule" &&
@@ -472,9 +472,9 @@ test_expect_success 'file vs modified submodule' '
 	git submodule update -N &&
 	test_must_fail git merge test$test_count &&
 	test -n "$(git ls-files -u)" &&
-	( yes "" | git mergetool file1 file2 spaced\ name subdir/file3 >/dev/null 2>&1 ) &&
-	( yes "" | git mergetool both >/dev/null 2>&1 ) &&
-	( yes "d" | git mergetool file11 file12 >/dev/null 2>&1 ) &&
+	( yes "" | git mergetool file1 file2 spaced\ name subdir/file3 ) &&
+	( yes "" | git mergetool both ) &&
+	( yes "d" | git mergetool file11 file12 ) &&
 	( yes "r" | git mergetool submod ) &&
 	test -d submod.orig &&
 	git submodule update -N &&
@@ -488,9 +488,9 @@ test_expect_success 'file vs modified submodule' '
 	git submodule update -N &&
 	test_must_fail git merge test$test_count &&
 	test -n "$(git ls-files -u)" &&
-	( yes "" | git mergetool file1 file2 spaced\ name subdir/file3 >/dev/null 2>&1 ) &&
-	( yes "" | git mergetool both>/dev/null 2>&1 ) &&
-	( yes "d" | git mergetool file11 file12 >/dev/null 2>&1 ) &&
+	( yes "" | git mergetool file1 file2 spaced\ name subdir/file3 ) &&
+	( yes "" | git mergetool both ) &&
+	( yes "d" | git mergetool file11 file12 ) &&
 	( yes "l" | git mergetool submod ) &&
 	test "$(cat submod/bar)" = "master submodule" &&
 	git submodule update -N &&
@@ -543,7 +543,7 @@ test_expect_success 'submodule in subdirectory' '
 	git add subdir/subdir_module &&
 	git commit -m "change submodule in subdirectory on test$test_count.b" &&
 
-	test_must_fail git merge test$test_count.a >/dev/null 2>&1 &&
+	test_must_fail git merge test$test_count.a &&
 	(
 		cd subdir &&
 		( yes "l" | git mergetool subdir_module )
@@ -554,7 +554,7 @@ test_expect_success 'submodule in subdirectory' '
 	git reset --hard &&
 	git submodule update -N &&
 
-	test_must_fail git merge test$test_count.a >/dev/null 2>&1 &&
+	test_must_fail git merge test$test_count.a &&
 	( yes "r" | git mergetool subdir/subdir_module ) &&
 	test "$(cat subdir/subdir_module/file15)" = "test$test_count.b" &&
 	git submodule update -N &&
@@ -641,7 +641,7 @@ test_expect_success 'filenames seen by tools start with ./' '
 	test_config mergetool.myecho.trustExitCode true &&
 	test_must_fail git merge master &&
 	git mergetool --no-prompt --tool myecho -- both >actual &&
-	grep ^\./both_LOCAL_ actual >/dev/null
+	grep ^\./both_LOCAL_ actual
 '
 
 test_lazy_prereq MKTEMP '
@@ -658,8 +658,8 @@ test_expect_success MKTEMP 'temporary filenames are used with mergetool.writeToT
 	test_config mergetool.myecho.trustExitCode true &&
 	test_must_fail git merge master &&
 	git mergetool --no-prompt --tool myecho -- both >actual &&
-	! grep ^\./both_LOCAL_ actual >/dev/null &&
-	grep /both_LOCAL_ actual >/dev/null
+	! grep ^\./both_LOCAL_ actual &&
+	grep /both_LOCAL_ actual
 '
 
 test_expect_success 'diff.orderFile configuration is honored' '
-- 
2.21.0.1000.g7817e26e80


^ permalink raw reply related	[flat|nested] 48+ messages in thread

* [PATCH v3 2/6] t7610: add mergetool --gui tests
  2019-04-24 22:46   ` [PATCH v3 0/6] difftool and mergetool improvements Denton Liu
  2019-04-24 22:46     ` [PATCH v3 1/6] t7610: unsuppress output Denton Liu
@ 2019-04-24 22:46     ` Denton Liu
  2019-04-24 22:47     ` [PATCH v3 3/6] mergetool: use get_merge_tool function Denton Liu
                       ` (4 subsequent siblings)
  6 siblings, 0 replies; 48+ messages in thread
From: Denton Liu @ 2019-04-24 22:46 UTC (permalink / raw)
  To: Git Mailing List
  Cc: Johannes Schindelin, David Aguilar, Jeff Hostetler,
	Eric Sunshine, Junio C Hamano

In 063f2bdbf7 (mergetool: accept -g/--[no-]gui as arguments,
2018-10-24), mergetool was taught the --gui option but no tests were
added to ensure that it was working properly. Add a test to ensure that
it works.

Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
 t/t7610-mergetool.sh | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/t/t7610-mergetool.sh b/t/t7610-mergetool.sh
index 69711487dd..dad607e186 100755
--- a/t/t7610-mergetool.sh
+++ b/t/t7610-mergetool.sh
@@ -145,6 +145,28 @@ test_expect_success 'custom mergetool' '
 	git commit -m "branch1 resolved with mergetool"
 '
 
+test_expect_success 'gui mergetool' '
+	test_config merge.guitool myguitool &&
+	test_config mergetool.myguitool.cmd "(printf \"gui \" && cat \"\$REMOTE\") >\"\$MERGED\"" &&
+	test_config mergetool.myguitool.trustExitCode true &&
+	test_when_finished "git reset --hard" &&
+	git checkout -b test$test_count branch1 &&
+	git submodule update -N &&
+	test_must_fail git merge master &&
+	( yes "" | git mergetool --gui both ) &&
+	( yes "" | git mergetool -g file1 file1 ) &&
+	( yes "" | git mergetool --gui file2 "spaced name" ) &&
+	( yes "" | git mergetool --gui subdir/file3 ) &&
+	( yes "d" | git mergetool --gui file11 ) &&
+	( yes "d" | git mergetool --gui file12 ) &&
+	( yes "l" | git mergetool --gui submod ) &&
+	test "$(cat file1)" = "gui master updated" &&
+	test "$(cat file2)" = "gui master new" &&
+	test "$(cat subdir/file3)" = "gui master new sub" &&
+	test "$(cat submod/bar)" = "branch1 submodule" &&
+	git commit -m "branch1 resolved with mergetool"
+'
+
 test_expect_success 'mergetool crlf' '
 	test_when_finished "git reset --hard" &&
 	# This test_config line must go after the above reset line so that
-- 
2.21.0.1000.g7817e26e80


^ permalink raw reply related	[flat|nested] 48+ messages in thread

* [PATCH v3 3/6] mergetool: use get_merge_tool function
  2019-04-24 22:46   ` [PATCH v3 0/6] difftool and mergetool improvements Denton Liu
  2019-04-24 22:46     ` [PATCH v3 1/6] t7610: unsuppress output Denton Liu
  2019-04-24 22:46     ` [PATCH v3 2/6] t7610: add mergetool --gui tests Denton Liu
@ 2019-04-24 22:47     ` Denton Liu
  2019-04-25  2:36       ` Junio C Hamano
  2019-04-24 22:47     ` [PATCH v3 4/6] mergetool: fallback to tool when guitool unavailable Denton Liu
                       ` (3 subsequent siblings)
  6 siblings, 1 reply; 48+ messages in thread
From: Denton Liu @ 2019-04-24 22:47 UTC (permalink / raw)
  To: Git Mailing List
  Cc: Johannes Schindelin, David Aguilar, Jeff Hostetler,
	Eric Sunshine, Junio C Hamano

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(-)

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


^ permalink raw reply related	[flat|nested] 48+ messages in thread

* [PATCH v3 4/6] mergetool: fallback to tool when guitool unavailable
  2019-04-24 22:46   ` [PATCH v3 0/6] difftool and mergetool improvements Denton Liu
                       ` (2 preceding siblings ...)
  2019-04-24 22:47     ` [PATCH v3 3/6] mergetool: use get_merge_tool function Denton Liu
@ 2019-04-24 22:47     ` Denton Liu
  2019-04-25  3:02       ` Junio C Hamano
  2019-04-24 22:47     ` [PATCH v3 5/6] difftool: make --gui, --tool and --extcmd mutually exclusive Denton Liu
                       ` (2 subsequent siblings)
  6 siblings, 1 reply; 48+ messages in thread
From: Denton Liu @ 2019-04-24 22:47 UTC (permalink / raw)
  To: Git Mailing List
  Cc: Johannes Schindelin, David Aguilar, Jeff Hostetler,
	Eric Sunshine, Junio C Hamano

In git-difftool, if the tool is called with --gui but `diff.guitool` is
not set, it falls back to `diff.tool`. Make git-mergetool also fallback
from `merge.guitool` to `merge.tool` if the former is undefined.

If git-difftool were to use `get_configured_mergetool`, it would also
get the fallback behaviour in the following precedence:

1. diff.guitool
2. merge.guitool
3. diff.tool
4. merge.tool

Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
 Documentation/git-mergetool.txt |  4 +++-
 git-mergetool--lib.sh           | 27 ++++++++++++++++++---------
 t/t7610-mergetool.sh            | 19 +++++++++++++++++++
 3 files changed, 40 insertions(+), 10 deletions(-)

diff --git a/Documentation/git-mergetool.txt b/Documentation/git-mergetool.txt
index 0c7975a050..6b14702e78 100644
--- a/Documentation/git-mergetool.txt
+++ b/Documentation/git-mergetool.txt
@@ -83,7 +83,9 @@ success of the resolution after the custom tool has exited.
 --gui::
 	When 'git-mergetool' is invoked with the `-g` or `--gui` option
 	the default merge tool will be read from the configured
-	`merge.guitool` variable instead of `merge.tool`.
+	`merge.guitool` variable instead of `merge.tool`. If
+	`merge.guitool` is not set, we will fallback to the tool
+	configured under `merge.tool`.
 
 --no-gui::
 	This overrides a previous `-g` or `--gui` setting and reads the
diff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh
index 68ff26a0f7..ada16acffc 100644
--- a/git-mergetool--lib.sh
+++ b/git-mergetool--lib.sh
@@ -350,20 +350,29 @@ guess_merge_tool () {
 }
 
 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
-		gui_prefix=gui
+		sections="diff $sections"
 	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
 	then
-		merge_tool=$(git config diff.${gui_prefix}tool || git config merge.${gui_prefix}tool)
-	else
-		merge_tool=$(git config merge.${gui_prefix}tool)
+		keys="guitool $keys"
 	fi
+
+	IFS=' '
+	for key in $keys
+	do
+		for section in $sections
+		do
+			merge_tool=$(git config $section.$key) && break 2
+		done
+	done
+
 	if test -n "$merge_tool" && ! valid_tool "$merge_tool"
 	then
 		echo >&2 "git config option $TOOL_MODE.${gui_prefix}tool set to unknown tool: $merge_tool"
diff --git a/t/t7610-mergetool.sh b/t/t7610-mergetool.sh
index dad607e186..5b61c10a9c 100755
--- a/t/t7610-mergetool.sh
+++ b/t/t7610-mergetool.sh
@@ -167,6 +167,25 @@ test_expect_success 'gui mergetool' '
 	git commit -m "branch1 resolved with mergetool"
 '
 
+test_expect_success 'gui mergetool without merge.guitool set falls back to merge.tool' '
+	test_when_finished "git reset --hard" &&
+	git checkout -b test$test_count branch1 &&
+	git submodule update -N &&
+	test_must_fail git merge master &&
+	( yes "" | git mergetool --gui both ) &&
+	( yes "" | git mergetool -g file1 file1 ) &&
+	( yes "" | git mergetool --gui file2 "spaced name" ) &&
+	( yes "" | git mergetool --gui subdir/file3 ) &&
+	( yes "d" | git mergetool --gui file11 ) &&
+	( yes "d" | git mergetool --gui file12 ) &&
+	( yes "l" | git mergetool --gui submod ) &&
+	test "$(cat file1)" = "master updated" &&
+	test "$(cat file2)" = "master new" &&
+	test "$(cat subdir/file3)" = "master new sub" &&
+	test "$(cat submod/bar)" = "branch1 submodule" &&
+	git commit -m "branch1 resolved with mergetool"
+'
+
 test_expect_success 'mergetool crlf' '
 	test_when_finished "git reset --hard" &&
 	# This test_config line must go after the above reset line so that
-- 
2.21.0.1000.g7817e26e80


^ permalink raw reply related	[flat|nested] 48+ messages in thread

* [PATCH v3 5/6] difftool: make --gui, --tool and --extcmd mutually exclusive
  2019-04-24 22:46   ` [PATCH v3 0/6] difftool and mergetool improvements Denton Liu
                       ` (3 preceding siblings ...)
  2019-04-24 22:47     ` [PATCH v3 4/6] mergetool: fallback to tool when guitool unavailable Denton Liu
@ 2019-04-24 22:47     ` Denton Liu
  2019-04-24 22:47     ` [PATCH v3 6/6] difftool: fallback on merge.guitool Denton Liu
  2019-04-25  9:54     ` [PATCH v4 0/6] difftool and mergetool improvements Denton Liu
  6 siblings, 0 replies; 48+ messages in thread
From: Denton Liu @ 2019-04-24 22:47 UTC (permalink / raw)
  To: Git Mailing List
  Cc: Johannes Schindelin, David Aguilar, Jeff Hostetler,
	Eric Sunshine, Junio C Hamano

In git-difftool, these options specify which tool to ultimately run. As
a result, they are logically conflicting. Explicitly disallow these
options from being used together.

Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
 builtin/difftool.c  | 3 +++
 t/t7800-difftool.sh | 8 ++++++++
 2 files changed, 11 insertions(+)

diff --git a/builtin/difftool.c b/builtin/difftool.c
index a3ea60ea71..65bba90338 100644
--- a/builtin/difftool.c
+++ b/builtin/difftool.c
@@ -731,6 +731,9 @@ int cmd_difftool(int argc, const char **argv, const char *prefix)
 	setenv(GIT_DIR_ENVIRONMENT, absolute_path(get_git_dir()), 1);
 	setenv(GIT_WORK_TREE_ENVIRONMENT, absolute_path(get_git_work_tree()), 1);
 
+	if (use_gui_tool + !!difftool_cmd + !!extcmd > 1)
+		die(_("--gui, --tool and --extcmd are mutually exclusive"));
+
 	if (use_gui_tool && diff_gui_tool && *diff_gui_tool)
 		setenv("GIT_DIFF_TOOL", diff_gui_tool, 1);
 	else if (difftool_cmd) {
diff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh
index bb9a7f4ff9..107f31213d 100755
--- a/t/t7800-difftool.sh
+++ b/t/t7800-difftool.sh
@@ -705,4 +705,12 @@ test_expect_success SYMLINKS 'difftool --dir-diff handles modified symlinks' '
 	test_cmp expect actual
 '
 
+test_expect_success 'difftool --gui, --tool and --extcmd are exclusive' '
+	difftool_test_setup &&
+	test_must_fail git difftool --gui --tool=test-tool &&
+	test_must_fail git difftool --gui --extcmd=cat &&
+	test_must_fail git difftool --tool=test-tool --extcmd=cat &&
+	test_must_fail git difftool --gui --tool=test-tool --extcmd=cat
+'
+
 test_done
-- 
2.21.0.1000.g7817e26e80


^ permalink raw reply related	[flat|nested] 48+ messages in thread

* [PATCH v3 6/6] difftool: fallback on merge.guitool
  2019-04-24 22:46   ` [PATCH v3 0/6] difftool and mergetool improvements Denton Liu
                       ` (4 preceding siblings ...)
  2019-04-24 22:47     ` [PATCH v3 5/6] difftool: make --gui, --tool and --extcmd mutually exclusive Denton Liu
@ 2019-04-24 22:47     ` Denton Liu
  2019-04-25  3:10       ` Junio C Hamano
  2019-04-25  9:54     ` [PATCH v4 0/6] difftool and mergetool improvements Denton Liu
  6 siblings, 1 reply; 48+ messages in thread
From: Denton Liu @ 2019-04-24 22:47 UTC (permalink / raw)
  To: Git Mailing List
  Cc: Johannes Schindelin, David Aguilar, Jeff Hostetler,
	Eric Sunshine, Junio C Hamano

In git-difftool.txt, it says

	'git difftool' falls back to 'git mergetool' config variables when the
	difftool equivalents have not been defined.

However, when `diff.guitool` is missing, it doesn't fallback to
anything. Make git-difftool fallback to `merge.guitool` when `diff.guitool` is
missing.

Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
 Documentation/git-difftool.txt |  4 +++-
 builtin/difftool.c             | 10 ++--------
 t/t7800-difftool.sh            | 16 ++++++++++++++++
 3 files changed, 21 insertions(+), 9 deletions(-)

diff --git a/Documentation/git-difftool.txt b/Documentation/git-difftool.txt
index 96c26e6aa8..484c485fd0 100644
--- a/Documentation/git-difftool.txt
+++ b/Documentation/git-difftool.txt
@@ -90,7 +90,9 @@ instead.  `--no-symlinks` is the default on Windows.
 	When 'git-difftool' is invoked with the `-g` or `--gui` option
 	the default diff tool will be read from the configured
 	`diff.guitool` variable instead of `diff.tool`. The `--no-gui`
-	option can be used to override this setting.
+	option can be used to override this setting. If `diff.guitool`
+	is not set, we will fallback in the order of `merge.guitool`,
+	`diff.tool`, `merge.tool` until a tool is found.
 
 --[no-]trust-exit-code::
 	'git-difftool' invokes a diff tool individually on each file.
diff --git a/builtin/difftool.c b/builtin/difftool.c
index 65bba90338..10660639c0 100644
--- a/builtin/difftool.c
+++ b/builtin/difftool.c
@@ -24,7 +24,6 @@
 #include "object-store.h"
 #include "dir.h"
 
-static char *diff_gui_tool;
 static int trust_exit_code;
 
 static const char *const builtin_difftool_usage[] = {
@@ -34,11 +33,6 @@ static const char *const builtin_difftool_usage[] = {
 
 static int difftool_config(const char *var, const char *value, void *cb)
 {
-	if (!strcmp(var, "diff.guitool")) {
-		diff_gui_tool = xstrdup(value);
-		return 0;
-	}
-
 	if (!strcmp(var, "difftool.trustexitcode")) {
 		trust_exit_code = git_config_bool(var, value);
 		return 0;
@@ -734,8 +728,8 @@ int cmd_difftool(int argc, const char **argv, const char *prefix)
 	if (use_gui_tool + !!difftool_cmd + !!extcmd > 1)
 		die(_("--gui, --tool and --extcmd are mutually exclusive"));
 
-	if (use_gui_tool && diff_gui_tool && *diff_gui_tool)
-		setenv("GIT_DIFF_TOOL", diff_gui_tool, 1);
+	if (use_gui_tool)
+		setenv("GIT_MERGETOOL_GUI", "true", 1);
 	else if (difftool_cmd) {
 		if (*difftool_cmd)
 			setenv("GIT_DIFF_TOOL", difftool_cmd, 1);
diff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh
index 107f31213d..ae90701a12 100755
--- a/t/t7800-difftool.sh
+++ b/t/t7800-difftool.sh
@@ -279,11 +279,27 @@ test_expect_success 'difftool + mergetool config variables' '
 	echo branch >expect &&
 	git difftool --no-prompt branch >actual &&
 	test_cmp expect actual &&
+	git difftool --gui --no-prompt branch >actual &&
+	test_cmp expect actual &&
 
 	# set merge.tool to something bogus, diff.tool to test-tool
 	test_config merge.tool bogus-tool &&
 	test_config diff.tool test-tool &&
 	git difftool --no-prompt branch >actual &&
+	test_cmp expect actual &&
+	git difftool --gui --no-prompt branch >actual &&
+	test_cmp expect actual &&
+
+	# set merge.tool, diff.tool to something bogus, merge.guitool to test-tool
+	test_config diff.tool bogus-tool &&
+	test_config merge.guitool test-tool &&
+	git difftool --gui --no-prompt branch >actual &&
+	test_cmp expect actual &&
+
+	# set merge.tool, diff.tool, merge.guitool to something bogus, diff.guitool to test-tool
+	test_config merge.guitool bogus-tool &&
+	test_config diff.guitool test-tool &&
+	git difftool --gui --no-prompt branch >actual &&
 	test_cmp expect actual
 '
 
-- 
2.21.0.1000.g7817e26e80


^ permalink raw reply related	[flat|nested] 48+ messages in thread

* Re: [PATCH v3 1/6] t7610: unsuppress output
  2019-04-24 22:46     ` [PATCH v3 1/6] t7610: unsuppress output Denton Liu
@ 2019-04-25  2:31       ` Junio C Hamano
  0 siblings, 0 replies; 48+ messages in thread
From: Junio C Hamano @ 2019-04-25  2:31 UTC (permalink / raw)
  To: Denton Liu
  Cc: Git Mailing List, Johannes Schindelin, David Aguilar,
	Jeff Hostetler, Eric Sunshine

Denton Liu <liu.denton@gmail.com> writes:

> The output for commands used to be suppressed by redirecting both stdout
> and stderr to /dev/null. However, this should not happen since the
> output is useful for debugging and, without the "-v" flag, test scripts
> don't output anyway.
>
> Unsuppress the output by removing the redirections to /dev/null.

Thanks for clearly justifying the clean-up.

I take it that with this there is no behaviour changes (e.g. a
command that may change its behaviour depending on where its
standard output goes does not change the outcome of the tests)?

I am not worried about the invocations of "git mergetool" downstream
of the pipe that is fed by "yes", and the remainder are a handful of
"git merge" invocations, which looked OK to me.

Thanks.

> Signed-off-by: Denton Liu <liu.denton@gmail.com>
> ---
>  t/t7610-mergetool.sh | 122 +++++++++++++++++++++----------------------
>  1 file changed, 61 insertions(+), 61 deletions(-)
>
> diff --git a/t/t7610-mergetool.sh b/t/t7610-mergetool.sh
> index a9fb971615..69711487dd 100755
> --- a/t/t7610-mergetool.sh
> +++ b/t/t7610-mergetool.sh
> @@ -130,14 +130,14 @@ test_expect_success 'custom mergetool' '
>  	test_when_finished "git reset --hard" &&
>  	git checkout -b test$test_count branch1 &&
>  	git submodule update -N &&
> -	test_must_fail git merge master >/dev/null 2>&1 &&
> -	( yes "" | git mergetool both >/dev/null 2>&1 ) &&
> +	test_must_fail git merge master &&
> +	( yes "" | git mergetool both ) &&
>  	( yes "" | git mergetool file1 file1 ) &&
> -	( yes "" | git mergetool file2 "spaced name" >/dev/null 2>&1 ) &&
> -	( yes "" | git mergetool subdir/file3 >/dev/null 2>&1 ) &&
> -	( yes "d" | git mergetool file11 >/dev/null 2>&1 ) &&
> -	( yes "d" | git mergetool file12 >/dev/null 2>&1 ) &&
> -	( yes "l" | git mergetool submod >/dev/null 2>&1 ) &&
> +	( yes "" | git mergetool file2 "spaced name" ) &&
> +	( yes "" | git mergetool subdir/file3 ) &&
> +	( yes "d" | git mergetool file11 ) &&
> +	( yes "d" | git mergetool file12 ) &&
> +	( yes "l" | git mergetool submod ) &&
>  	test "$(cat file1)" = "master updated" &&
>  	test "$(cat file2)" = "master new" &&
>  	test "$(cat subdir/file3)" = "master new sub" &&
> @@ -153,15 +153,15 @@ test_expect_success 'mergetool crlf' '
>  	# test_when_finished is LIFO.)
>  	test_config core.autocrlf true &&
>  	git checkout -b test$test_count branch1 &&
> -	test_must_fail git merge master >/dev/null 2>&1 &&
> -	( yes "" | git mergetool file1 >/dev/null 2>&1 ) &&
> -	( yes "" | git mergetool file2 >/dev/null 2>&1 ) &&
> -	( yes "" | git mergetool "spaced name" >/dev/null 2>&1 ) &&
> -	( yes "" | git mergetool both >/dev/null 2>&1 ) &&
> -	( yes "" | git mergetool subdir/file3 >/dev/null 2>&1 ) &&
> -	( yes "d" | git mergetool file11 >/dev/null 2>&1 ) &&
> -	( yes "d" | git mergetool file12 >/dev/null 2>&1 ) &&
> -	( yes "r" | git mergetool submod >/dev/null 2>&1 ) &&
> +	test_must_fail git merge master &&
> +	( yes "" | git mergetool file1 ) &&
> +	( yes "" | git mergetool file2 ) &&
> +	( yes "" | git mergetool "spaced name" ) &&
> +	( yes "" | git mergetool both ) &&
> +	( yes "" | git mergetool subdir/file3 ) &&
> +	( yes "d" | git mergetool file11 ) &&
> +	( yes "d" | git mergetool file12 ) &&
> +	( yes "r" | git mergetool submod ) &&
>  	test "$(printf x | cat file1 -)" = "$(printf "master updated\r\nx")" &&
>  	test "$(printf x | cat file2 -)" = "$(printf "master new\r\nx")" &&
>  	test "$(printf x | cat subdir/file3 -)" = "$(printf "master new sub\r\nx")" &&
> @@ -176,8 +176,8 @@ test_expect_success 'mergetool in subdir' '
>  	git submodule update -N &&
>  	(
>  		cd subdir &&
> -		test_must_fail git merge master >/dev/null 2>&1 &&
> -		( yes "" | git mergetool file3 >/dev/null 2>&1 ) &&
> +		test_must_fail git merge master &&
> +		( yes "" | git mergetool file3 ) &&
>  		test "$(cat file3)" = "master new sub"
>  	)
>  '
> @@ -188,14 +188,14 @@ test_expect_success 'mergetool on file in parent dir' '
>  	git submodule update -N &&
>  	(
>  		cd subdir &&
> -		test_must_fail git merge master >/dev/null 2>&1 &&
> -		( yes "" | git mergetool file3 >/dev/null 2>&1 ) &&
> -		( yes "" | git mergetool ../file1 >/dev/null 2>&1 ) &&
> -		( yes "" | git mergetool ../file2 ../spaced\ name >/dev/null 2>&1 ) &&
> -		( yes "" | git mergetool ../both >/dev/null 2>&1 ) &&
> -		( yes "d" | git mergetool ../file11 >/dev/null 2>&1 ) &&
> -		( yes "d" | git mergetool ../file12 >/dev/null 2>&1 ) &&
> -		( yes "l" | git mergetool ../submod >/dev/null 2>&1 ) &&
> +		test_must_fail git merge master &&
> +		( yes "" | git mergetool file3 ) &&
> +		( yes "" | git mergetool ../file1 ) &&
> +		( yes "" | git mergetool ../file2 ../spaced\ name ) &&
> +		( yes "" | git mergetool ../both ) &&
> +		( yes "d" | git mergetool ../file11 ) &&
> +		( yes "d" | git mergetool ../file12 ) &&
> +		( yes "l" | git mergetool ../submod ) &&
>  		test "$(cat ../file1)" = "master updated" &&
>  		test "$(cat ../file2)" = "master new" &&
>  		test "$(cat ../submod/bar)" = "branch1 submodule" &&
> @@ -209,9 +209,9 @@ test_expect_success 'mergetool skips autoresolved' '
>  	git submodule update -N &&
>  	test_must_fail git merge master &&
>  	test -n "$(git ls-files -u)" &&
> -	( yes "d" | git mergetool file11 >/dev/null 2>&1 ) &&
> -	( yes "d" | git mergetool file12 >/dev/null 2>&1 ) &&
> -	( yes "l" | git mergetool submod >/dev/null 2>&1 ) &&
> +	( yes "d" | git mergetool file11 ) &&
> +	( yes "d" | git mergetool file12 ) &&
> +	( yes "l" | git mergetool submod ) &&
>  	output="$(git mergetool --no-prompt)" &&
>  	test "$output" = "No files need merging"
>  '
> @@ -259,9 +259,9 @@ test_expect_success 'mergetool skips resolved paths when rerere is active' '
>  	rm -rf .git/rr-cache &&
>  	git checkout -b test$test_count branch1 &&
>  	git submodule update -N &&
> -	test_must_fail git merge master >/dev/null 2>&1 &&
> -	( yes "l" | git mergetool --no-prompt submod >/dev/null 2>&1 ) &&
> -	( yes "d" "d" | git mergetool --no-prompt >/dev/null 2>&1 ) &&
> +	test_must_fail git merge master &&
> +	( yes "l" | git mergetool --no-prompt submod ) &&
> +	( yes "d" "d" | git mergetool --no-prompt ) &&
>  	git submodule update -N &&
>  	output="$(yes "n" | git mergetool --no-prompt)" &&
>  	test "$output" = "No files need merging"
> @@ -369,9 +369,9 @@ test_expect_success 'deleted vs modified submodule' '
>  	git checkout -b test$test_count.a test$test_count &&
>  	test_must_fail git merge master &&
>  	test -n "$(git ls-files -u)" &&
> -	( yes "" | git mergetool file1 file2 spaced\ name subdir/file3 >/dev/null 2>&1 ) &&
> -	( yes "" | git mergetool both >/dev/null 2>&1 ) &&
> -	( yes "d" | git mergetool file11 file12 >/dev/null 2>&1 ) &&
> +	( yes "" | git mergetool file1 file2 spaced\ name subdir/file3 ) &&
> +	( yes "" | git mergetool both ) &&
> +	( yes "d" | git mergetool file11 file12 ) &&
>  	( yes "r" | git mergetool submod ) &&
>  	rmdir submod && mv submod-movedaside submod &&
>  	test "$(cat submod/bar)" = "branch1 submodule" &&
> @@ -386,9 +386,9 @@ test_expect_success 'deleted vs modified submodule' '
>  	git submodule update -N &&
>  	test_must_fail git merge master &&
>  	test -n "$(git ls-files -u)" &&
> -	( yes "" | git mergetool file1 file2 spaced\ name subdir/file3 >/dev/null 2>&1 ) &&
> -	( yes "" | git mergetool both >/dev/null 2>&1 ) &&
> -	( yes "d" | git mergetool file11 file12 >/dev/null 2>&1 ) &&
> +	( yes "" | git mergetool file1 file2 spaced\ name subdir/file3 ) &&
> +	( yes "" | git mergetool both ) &&
> +	( yes "d" | git mergetool file11 file12 ) &&
>  	( yes "l" | git mergetool submod ) &&
>  	test ! -e submod &&
>  	output="$(git mergetool --no-prompt)" &&
> @@ -400,9 +400,9 @@ test_expect_success 'deleted vs modified submodule' '
>  	git submodule update -N &&
>  	test_must_fail git merge test$test_count &&
>  	test -n "$(git ls-files -u)" &&
> -	( yes "" | git mergetool file1 file2 spaced\ name subdir/file3 >/dev/null 2>&1 ) &&
> -	( yes "" | git mergetool both >/dev/null 2>&1 ) &&
> -	( yes "d" | git mergetool file11 file12 >/dev/null 2>&1 ) &&
> +	( yes "" | git mergetool file1 file2 spaced\ name subdir/file3 ) &&
> +	( yes "" | git mergetool both ) &&
> +	( yes "d" | git mergetool file11 file12 ) &&
>  	( yes "r" | git mergetool submod ) &&
>  	test ! -e submod &&
>  	test -d submod.orig &&
> @@ -416,9 +416,9 @@ test_expect_success 'deleted vs modified submodule' '
>  	git submodule update -N &&
>  	test_must_fail git merge test$test_count &&
>  	test -n "$(git ls-files -u)" &&
> -	( yes "" | git mergetool file1 file2 spaced\ name subdir/file3 >/dev/null 2>&1 ) &&
> -	( yes "" | git mergetool both >/dev/null 2>&1 ) &&
> -	( yes "d" | git mergetool file11 file12 >/dev/null 2>&1 ) &&
> +	( yes "" | git mergetool file1 file2 spaced\ name subdir/file3 ) &&
> +	( yes "" | git mergetool both ) &&
> +	( yes "d" | git mergetool file11 file12 ) &&
>  	( yes "l" | git mergetool submod ) &&
>  	test "$(cat submod/bar)" = "master submodule" &&
>  	git submodule update -N &&
> @@ -440,9 +440,9 @@ test_expect_success 'file vs modified submodule' '
>  	git checkout -b test$test_count.a branch1 &&
>  	test_must_fail git merge master &&
>  	test -n "$(git ls-files -u)" &&
> -	( yes "" | git mergetool file1 file2 spaced\ name subdir/file3 >/dev/null 2>&1 ) &&
> -	( yes "" | git mergetool both >/dev/null 2>&1 ) &&
> -	( yes "d" | git mergetool file11 file12 >/dev/null 2>&1 ) &&
> +	( yes "" | git mergetool file1 file2 spaced\ name subdir/file3 ) &&
> +	( yes "" | git mergetool both ) &&
> +	( yes "d" | git mergetool file11 file12 ) &&
>  	( yes "r" | git mergetool submod ) &&
>  	rmdir submod && mv submod-movedaside submod &&
>  	test "$(cat submod/bar)" = "branch1 submodule" &&
> @@ -456,9 +456,9 @@ test_expect_success 'file vs modified submodule' '
>  	git checkout -b test$test_count.b test$test_count &&
>  	test_must_fail git merge master &&
>  	test -n "$(git ls-files -u)" &&
> -	( yes "" | git mergetool file1 file2 spaced\ name subdir/file3 >/dev/null 2>&1 ) &&
> -	( yes "" | git mergetool both >/dev/null 2>&1 ) &&
> -	( yes "d" | git mergetool file11 file12 >/dev/null 2>&1 ) &&
> +	( yes "" | git mergetool file1 file2 spaced\ name subdir/file3 ) &&
> +	( yes "" | git mergetool both ) &&
> +	( yes "d" | git mergetool file11 file12 ) &&
>  	( yes "l" | git mergetool submod ) &&
>  	git submodule update -N &&
>  	test "$(cat submod)" = "not a submodule" &&
> @@ -472,9 +472,9 @@ test_expect_success 'file vs modified submodule' '
>  	git submodule update -N &&
>  	test_must_fail git merge test$test_count &&
>  	test -n "$(git ls-files -u)" &&
> -	( yes "" | git mergetool file1 file2 spaced\ name subdir/file3 >/dev/null 2>&1 ) &&
> -	( yes "" | git mergetool both >/dev/null 2>&1 ) &&
> -	( yes "d" | git mergetool file11 file12 >/dev/null 2>&1 ) &&
> +	( yes "" | git mergetool file1 file2 spaced\ name subdir/file3 ) &&
> +	( yes "" | git mergetool both ) &&
> +	( yes "d" | git mergetool file11 file12 ) &&
>  	( yes "r" | git mergetool submod ) &&
>  	test -d submod.orig &&
>  	git submodule update -N &&
> @@ -488,9 +488,9 @@ test_expect_success 'file vs modified submodule' '
>  	git submodule update -N &&
>  	test_must_fail git merge test$test_count &&
>  	test -n "$(git ls-files -u)" &&
> -	( yes "" | git mergetool file1 file2 spaced\ name subdir/file3 >/dev/null 2>&1 ) &&
> -	( yes "" | git mergetool both>/dev/null 2>&1 ) &&
> -	( yes "d" | git mergetool file11 file12 >/dev/null 2>&1 ) &&
> +	( yes "" | git mergetool file1 file2 spaced\ name subdir/file3 ) &&
> +	( yes "" | git mergetool both ) &&
> +	( yes "d" | git mergetool file11 file12 ) &&
>  	( yes "l" | git mergetool submod ) &&
>  	test "$(cat submod/bar)" = "master submodule" &&
>  	git submodule update -N &&
> @@ -543,7 +543,7 @@ test_expect_success 'submodule in subdirectory' '
>  	git add subdir/subdir_module &&
>  	git commit -m "change submodule in subdirectory on test$test_count.b" &&
>  
> -	test_must_fail git merge test$test_count.a >/dev/null 2>&1 &&
> +	test_must_fail git merge test$test_count.a &&
>  	(
>  		cd subdir &&
>  		( yes "l" | git mergetool subdir_module )
> @@ -554,7 +554,7 @@ test_expect_success 'submodule in subdirectory' '
>  	git reset --hard &&
>  	git submodule update -N &&
>  
> -	test_must_fail git merge test$test_count.a >/dev/null 2>&1 &&
> +	test_must_fail git merge test$test_count.a &&
>  	( yes "r" | git mergetool subdir/subdir_module ) &&
>  	test "$(cat subdir/subdir_module/file15)" = "test$test_count.b" &&
>  	git submodule update -N &&
> @@ -641,7 +641,7 @@ test_expect_success 'filenames seen by tools start with ./' '
>  	test_config mergetool.myecho.trustExitCode true &&
>  	test_must_fail git merge master &&
>  	git mergetool --no-prompt --tool myecho -- both >actual &&
> -	grep ^\./both_LOCAL_ actual >/dev/null
> +	grep ^\./both_LOCAL_ actual
>  '
>  
>  test_lazy_prereq MKTEMP '
> @@ -658,8 +658,8 @@ test_expect_success MKTEMP 'temporary filenames are used with mergetool.writeToT
>  	test_config mergetool.myecho.trustExitCode true &&
>  	test_must_fail git merge master &&
>  	git mergetool --no-prompt --tool myecho -- both >actual &&
> -	! grep ^\./both_LOCAL_ actual >/dev/null &&
> -	grep /both_LOCAL_ actual >/dev/null
> +	! grep ^\./both_LOCAL_ actual &&
> +	grep /both_LOCAL_ actual
>  '
>  
>  test_expect_success 'diff.orderFile configuration is honored' '

^ permalink raw reply	[flat|nested] 48+ messages in thread

* Re: [PATCH v3 3/6] mergetool: use get_merge_tool function
  2019-04-24 22:47     ` [PATCH v3 3/6] mergetool: use get_merge_tool function Denton Liu
@ 2019-04-25  2:36       ` Junio C Hamano
  0 siblings, 0 replies; 48+ messages in thread
From: Junio C Hamano @ 2019-04-25  2:36 UTC (permalink / raw)
  To: Denton Liu
  Cc: Git Mailing List, Johannes Schindelin, David Aguilar,
	Jeff Hostetler, Eric Sunshine

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.

^ permalink raw reply	[flat|nested] 48+ messages in thread

* Re: [PATCH v3 4/6] mergetool: fallback to tool when guitool unavailable
  2019-04-24 22:47     ` [PATCH v3 4/6] mergetool: fallback to tool when guitool unavailable Denton Liu
@ 2019-04-25  3:02       ` Junio C Hamano
  2019-04-25  5:16         ` Denton Liu
  0 siblings, 1 reply; 48+ messages in thread
From: Junio C Hamano @ 2019-04-25  3:02 UTC (permalink / raw)
  To: Denton Liu
  Cc: Git Mailing List, Johannes Schindelin, David Aguilar,
	Jeff Hostetler, Eric Sunshine

Denton Liu <liu.denton@gmail.com> writes:

> In git-difftool, if the tool is called with --gui but `diff.guitool` is
> not set, it falls back to `diff.tool`. Make git-mergetool also fallback
> from `merge.guitool` to `merge.tool` if the former is undefined.
>
> If git-difftool were to use `get_configured_mergetool`, it would also

I agree that the precedence order below makes sense, but I am a bit
confused by "were to use" here.  Do you mean you'll make the change
to make difftool to look at mergetool configuraiton in a later step
in the series?  Or is there a way for the user to say "I want my
difftool to also pay attention to the mergetool configurations" (and
another "I do not want that" option)?  I'll come back to this later.

> get the fallback behaviour in the following precedence:
>
> 1. diff.guitool
> 2. merge.guitool
> 3. diff.tool
> 4. merge.tool
>
> Signed-off-by: Denton Liu <liu.denton@gmail.com>
> ---
>  Documentation/git-mergetool.txt |  4 +++-
>  git-mergetool--lib.sh           | 27 ++++++++++++++++++---------
>  t/t7610-mergetool.sh            | 19 +++++++++++++++++++
>  3 files changed, 40 insertions(+), 10 deletions(-)
>
> diff --git a/Documentation/git-mergetool.txt b/Documentation/git-mergetool.txt
> index 0c7975a050..6b14702e78 100644
> --- a/Documentation/git-mergetool.txt
> +++ b/Documentation/git-mergetool.txt
> @@ -83,7 +83,9 @@ success of the resolution after the custom tool has exited.
>  --gui::
>  	When 'git-mergetool' is invoked with the `-g` or `--gui` option
>  	the default merge tool will be read from the configured
> -	`merge.guitool` variable instead of `merge.tool`.
> +	`merge.guitool` variable instead of `merge.tool`. If
> +	`merge.guitool` is not set, we will fallback to the tool
> +	configured under `merge.tool`.

Makes sense.  So "mergetool --gui" from the command line would look
at guitool and then tool in the merge section.

> diff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh
> index 68ff26a0f7..ada16acffc 100644
> --- a/git-mergetool--lib.sh
> +++ b/git-mergetool--lib.sh
> @@ -350,20 +350,29 @@ guess_merge_tool () {
>  }
>  
>  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
> -		gui_prefix=gui
> +		sections="diff $sections"
>  	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
>  	then
> -		merge_tool=$(git config diff.${gui_prefix}tool || git config merge.${gui_prefix}tool)
> -	else
> -		merge_tool=$(git config merge.${gui_prefix}tool)
> +		keys="guitool $keys"
>  	fi

OK, so $sections is "diff merge" for difftool and just "merge" for mergetool.
$tool is "guitool tool" under "--gui", and just "tool" otherwise.

> +	IFS=' '
> +	for key in $keys
> +	do
> +		for section in $sections
> +		do
> +			merge_tool=$(git config $section.$key) && break 2
> +		done
> +	done

And you do up to four iterations to cover the combination in the
precedence order.  Which makes sense.

I am not sure about the wisdom of setting IFS here, though.

As far as I can see, both $keys and $sections do not take any
arbitrary values, but just the two (for each) values you know that
do not have any funny characters, so I am not sure what you are
trying to achieve by that (i.e. benefit is unclear).

As long as the get_configured_merge_tool function is called always
for string_emitted_to_stdout=$(that function), the updated setting
will not leak to the outside world so there is no risk to break its
callers, but it is not immediately obvious if helper functions
called in the remainder of this function are OK with the modified
value of IFS (i.e. safety is not obvious).

Now for the promised "come back to this later", I think you meant
"the get_configured_merge_tool function is already prepared to be
used from difftool in this step and when difftool starts to call it
here is what happens".  But I wonder if it makes the evolution of
the topic easier to follow if you defer it to a later step when you
actually make difftool to start calling it?  In other words, in this
step, your get_configured_merge_tool would look like

	sections=merge

	case "$1" in
	true)
		keys="guitool tool" ;;
	*)
		keys="tool" ;;
	esac

	for key in $keys
	do
		for section in $sections
		do
			merge_tool=$(git config ...) && break 2
		done
	done
	...

and then in a later step (6/6?), the unconditional setting of
sections to 'merge' would be updated so that in diff_mode, you'll
iterate over two sections.

I dunno.

^ permalink raw reply	[flat|nested] 48+ messages in thread

* Re: [PATCH v3 6/6] difftool: fallback on merge.guitool
  2019-04-24 22:47     ` [PATCH v3 6/6] difftool: fallback on merge.guitool Denton Liu
@ 2019-04-25  3:10       ` Junio C Hamano
  0 siblings, 0 replies; 48+ messages in thread
From: Junio C Hamano @ 2019-04-25  3:10 UTC (permalink / raw)
  To: Denton Liu
  Cc: Git Mailing List, Johannes Schindelin, David Aguilar,
	Jeff Hostetler, Eric Sunshine

Denton Liu <liu.denton@gmail.com> writes:

> @@ -734,8 +728,8 @@ int cmd_difftool(int argc, const char **argv, const char *prefix)
>  	if (use_gui_tool + !!difftool_cmd + !!extcmd > 1)
>  		die(_("--gui, --tool and --extcmd are mutually exclusive"));
>  
> -	if (use_gui_tool && diff_gui_tool && *diff_gui_tool)
> -		setenv("GIT_DIFF_TOOL", diff_gui_tool, 1);
> +	if (use_gui_tool)
> +		setenv("GIT_MERGETOOL_GUI", "true", 1);
>  	else if (difftool_cmd) {
>  		if (*difftool_cmd)
>  			setenv("GIT_DIFF_TOOL", difftool_cmd, 1);

So unless difftool_cmd is given explicitly, we'll let the scripted
difftool--helper to let merge_tool=$(get_merge_tool) to pick the
tool, which will use the same logic you wrote in the step 2/6.

OK, that makes sense.

What was preventing the get_configured_merge_tool updated in step
2/6 from getting called in difftool was the exporting of
GIT_DIFF_TOOL we see abovethat was removed by this step.

Which also makes sense.

> diff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh
> index 107f31213d..ae90701a12 100755
> --- a/t/t7800-difftool.sh
> +++ b/t/t7800-difftool.sh
> @@ -279,11 +279,27 @@ test_expect_success 'difftool + mergetool config variables' '
>  	echo branch >expect &&
>  	git difftool --no-prompt branch >actual &&
>  	test_cmp expect actual &&
> +	git difftool --gui --no-prompt branch >actual &&
> +	test_cmp expect actual &&
>  
>  	# set merge.tool to something bogus, diff.tool to test-tool
>  	test_config merge.tool bogus-tool &&
>  	test_config diff.tool test-tool &&
>  	git difftool --no-prompt branch >actual &&
> +	test_cmp expect actual &&
> +	git difftool --gui --no-prompt branch >actual &&
> +	test_cmp expect actual &&
> +
> +	# set merge.tool, diff.tool to something bogus, merge.guitool to test-tool
> +	test_config diff.tool bogus-tool &&
> +	test_config merge.guitool test-tool &&
> +	git difftool --gui --no-prompt branch >actual &&
> +	test_cmp expect actual &&
> +
> +	# set merge.tool, diff.tool, merge.guitool to something bogus, diff.guitool to test-tool
> +	test_config merge.guitool bogus-tool &&
> +	test_config diff.guitool test-tool &&
> +	git difftool --gui --no-prompt branch >actual &&
>  	test_cmp expect actual
>  '

^ permalink raw reply	[flat|nested] 48+ messages in thread

* Re: [PATCH v3 4/6] mergetool: fallback to tool when guitool unavailable
  2019-04-25  3:02       ` Junio C Hamano
@ 2019-04-25  5:16         ` Denton Liu
  0 siblings, 0 replies; 48+ messages in thread
From: Denton Liu @ 2019-04-25  5:16 UTC (permalink / raw)
  To: Junio C Hamano
  Cc: Git Mailing List, Johannes Schindelin, David Aguilar,
	Jeff Hostetler, Eric Sunshine

Hi Junio,

On Thu, Apr 25, 2019 at 12:02:23PM +0900, Junio C Hamano wrote:
> Denton Liu <liu.denton@gmail.com> writes:
> 
> > In git-difftool, if the tool is called with --gui but `diff.guitool` is
> > not set, it falls back to `diff.tool`. Make git-mergetool also fallback
> > from `merge.guitool` to `merge.tool` if the former is undefined.
> >
> > If git-difftool were to use `get_configured_mergetool`, it would also
> 
> I agree that the precedence order below makes sense, but I am a bit
> confused by "were to use" here.  Do you mean you'll make the change
> to make difftool to look at mergetool configuraiton in a later step
> in the series?  Or is there a way for the user to say "I want my
> difftool to also pay attention to the mergetool configurations" (and
> another "I do not want that" option)?  I'll come back to this later.

Correct, it means it'll be done in a future patch (i.e. 6/6).

I guess I wasn't fully clear in the message. I meant something like, "If
`git difftool --gui` were to use..." because difftool currently already
uses this function in the non-gui case.

[snip]
> 
> > +	IFS=' '
> > +	for key in $keys
> > +	do
> > +		for section in $sections
> > +		do
> > +			merge_tool=$(git config $section.$key) && break 2
> > +		done
> > +	done
> 
> And you do up to four iterations to cover the combination in the
> precedence order.  Which makes sense.
> 
> I am not sure about the wisdom of setting IFS here, though.
> 
> As far as I can see, both $keys and $sections do not take any
> arbitrary values, but just the two (for each) values you know that
> do not have any funny characters, so I am not sure what you are
> trying to achieve by that (i.e. benefit is unclear).

The reason why IFS is being set is because at the top of mergetool--lib,
we set IFS to '\n'. As a result, without setting IFS, the strings
won't parse properly into the for loop.

> 
> As long as the get_configured_merge_tool function is called always
> for string_emitted_to_stdout=$(that function), the updated setting
> will not leak to the outside world so there is no risk to break its
> callers, but it is not immediately obvious if helper functions
> called in the remainder of this function are OK with the modified
> value of IFS (i.e. safety is not obvious).

When I was writing this, I didn't realise that the value of IFS bleeds
out of this function. I'll reroll this to use a helper function just in
case.

> 
> Now for the promised "come back to this later", I think you meant
> "the get_configured_merge_tool function is already prepared to be
> used from difftool in this step and when difftool starts to call it
> here is what happens".  But I wonder if it makes the evolution of
> the topic easier to follow if you defer it to a later step when you
> actually make difftool to start calling it?  In other words, in this
> step, your get_configured_merge_tool would look like
> 
> 	sections=merge
> 
> 	case "$1" in
> 	true)
> 		keys="guitool tool" ;;
> 	*)
> 		keys="tool" ;;
> 	esac
> 
> 	for key in $keys
> 	do
> 		for section in $sections
> 		do
> 			merge_tool=$(git config ...) && break 2
> 		done
> 	done
> 	...
> 
> and then in a later step (6/6?), the unconditional setting of
> sections to 'merge' would be updated so that in diff_mode, you'll
> iterate over two sections.
> 
> I dunno.

As stated above, difftool currently uses this function in the non-gui
case. I think that clarifying the log message on my part should make it
easier to understand the evolution of this topic.

Thanks for the careful review,

Denton

^ permalink raw reply	[flat|nested] 48+ messages in thread

* [PATCH v4 0/6] difftool and mergetool improvements
  2019-04-24 22:46   ` [PATCH v3 0/6] difftool and mergetool improvements Denton Liu
                       ` (5 preceding siblings ...)
  2019-04-24 22:47     ` [PATCH v3 6/6] difftool: fallback on merge.guitool Denton Liu
@ 2019-04-25  9:54     ` Denton Liu
  2019-04-25  9:54       ` [PATCH v4 1/6] t7610: unsuppress output Denton Liu
                         ` (6 more replies)
  6 siblings, 7 replies; 48+ messages in thread
From: Denton Liu @ 2019-04-25  9:54 UTC (permalink / raw)
  To: Git Mailing List
  Cc: Johannes Schindelin, David Aguilar, Jeff Hostetler,
	Eric Sunshine, Junio C Hamano

Thanks for another review, Junio.

This should basically be the same as v3 except I've moved the IFS
assignment into a subshell so its value does not leak out.

I'd like the change in 4/6 to be reviewed carefully. t7610 and t7800
run properly with dash on My Machineة, but I couldn't find a reference
on whether my usage is POSIX legal or not.

We do have precedent for using fors within a subshell, though, from a
quick git grep:

	t/t3202-show-branch-octopus.sh: git show-branch $(for i in $numbers; do echo branch$i; done) > out &&

Thanks,

Denton

---

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 (6):
  t7610: unsuppress output
  t7610: add mergetool --gui tests
  mergetool: use get_merge_tool 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                |  37 ++++--
 git-mergetool.sh                     |  12 +-
 t/t7610-mergetool.sh                 | 163 +++++++++++++++++----------
 t/t7800-difftool.sh                  |  24 ++++
 9 files changed, 172 insertions(+), 91 deletions(-)

Range-diff against v3:
1:  9f9922cab3 = 1:  9f9922cab3 t7610: unsuppress output
2:  0f632ca6bf = 2:  0f632ca6bf t7610: add mergetool --gui tests
3:  ff83d25ff2 = 3:  ff83d25ff2 mergetool: use get_merge_tool function
4:  e975fe4a8b ! 4:  c799e871e2 mergetool: fallback to tool when guitool unavailable
    @@ -6,14 +6,18 @@
         not set, it falls back to `diff.tool`. Make git-mergetool also fallback
         from `merge.guitool` to `merge.tool` if the former is undefined.
     
    -    If git-difftool were to use `get_configured_mergetool`, it would also
    -    get the fallback behaviour in the following precedence:
    +    If git-difftool, when called with `--gui`, were to use
    +    `get_configured_mergetool` in a future patch, it would also get the
    +    fallback behavior in the following precedence:
     
         1. diff.guitool
         2. merge.guitool
         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.
    +
         Signed-off-by: Denton Liu <liu.denton@gmail.com>
     
      diff --git a/Documentation/git-mergetool.txt b/Documentation/git-mergetool.txt
    @@ -61,14 +65,19 @@
     +		keys="guitool $keys"
      	fi
     +
    -+	IFS=' '
    -+	for key in $keys
    -+	do
    -+		for section in $sections
    ++	merge_tool=$(
    ++		IFS=' '
    ++		for key in $keys
     +		do
    -+			merge_tool=$(git config $section.$key) && break 2
    -+		done
    -+	done
    ++			for section in $sections
    ++			do
    ++				if selected=$(git config $section.$key)
    ++				then
    ++					echo "$selected"
    ++					return
    ++				fi
    ++			done
    ++		done)
     +
      	if test -n "$merge_tool" && ! valid_tool "$merge_tool"
      	then
5:  bc3e229171 = 5:  7d7c936cd3 difftool: make --gui, --tool and --extcmd mutually exclusive
6:  f39b15efbd = 6:  b642ed3b1e difftool: fallback on merge.guitool
-- 
2.21.0.1000.g11cd861522


^ permalink raw reply	[flat|nested] 48+ messages in thread

* [PATCH v4 1/6] t7610: unsuppress output
  2019-04-25  9:54     ` [PATCH v4 0/6] difftool and mergetool improvements Denton Liu
@ 2019-04-25  9:54       ` Denton Liu
  2019-04-25  9:54       ` [PATCH v4 2/6] t7610: add mergetool --gui tests Denton Liu
                         ` (5 subsequent siblings)
  6 siblings, 0 replies; 48+ messages in thread
From: Denton Liu @ 2019-04-25  9:54 UTC (permalink / raw)
  To: Git Mailing List
  Cc: Johannes Schindelin, David Aguilar, Jeff Hostetler,
	Eric Sunshine, Junio C Hamano

The output for commands used to be suppressed by redirecting both stdout
and stderr to /dev/null. However, this should not happen since the
output is useful for debugging and, without the "-v" flag, test scripts
don't output anyway.

Unsuppress the output by removing the redirections to /dev/null.

Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
 t/t7610-mergetool.sh | 122 +++++++++++++++++++++----------------------
 1 file changed, 61 insertions(+), 61 deletions(-)

diff --git a/t/t7610-mergetool.sh b/t/t7610-mergetool.sh
index a9fb971615..69711487dd 100755
--- a/t/t7610-mergetool.sh
+++ b/t/t7610-mergetool.sh
@@ -130,14 +130,14 @@ test_expect_success 'custom mergetool' '
 	test_when_finished "git reset --hard" &&
 	git checkout -b test$test_count branch1 &&
 	git submodule update -N &&
-	test_must_fail git merge master >/dev/null 2>&1 &&
-	( yes "" | git mergetool both >/dev/null 2>&1 ) &&
+	test_must_fail git merge master &&
+	( yes "" | git mergetool both ) &&
 	( yes "" | git mergetool file1 file1 ) &&
-	( yes "" | git mergetool file2 "spaced name" >/dev/null 2>&1 ) &&
-	( yes "" | git mergetool subdir/file3 >/dev/null 2>&1 ) &&
-	( yes "d" | git mergetool file11 >/dev/null 2>&1 ) &&
-	( yes "d" | git mergetool file12 >/dev/null 2>&1 ) &&
-	( yes "l" | git mergetool submod >/dev/null 2>&1 ) &&
+	( yes "" | git mergetool file2 "spaced name" ) &&
+	( yes "" | git mergetool subdir/file3 ) &&
+	( yes "d" | git mergetool file11 ) &&
+	( yes "d" | git mergetool file12 ) &&
+	( yes "l" | git mergetool submod ) &&
 	test "$(cat file1)" = "master updated" &&
 	test "$(cat file2)" = "master new" &&
 	test "$(cat subdir/file3)" = "master new sub" &&
@@ -153,15 +153,15 @@ test_expect_success 'mergetool crlf' '
 	# test_when_finished is LIFO.)
 	test_config core.autocrlf true &&
 	git checkout -b test$test_count branch1 &&
-	test_must_fail git merge master >/dev/null 2>&1 &&
-	( yes "" | git mergetool file1 >/dev/null 2>&1 ) &&
-	( yes "" | git mergetool file2 >/dev/null 2>&1 ) &&
-	( yes "" | git mergetool "spaced name" >/dev/null 2>&1 ) &&
-	( yes "" | git mergetool both >/dev/null 2>&1 ) &&
-	( yes "" | git mergetool subdir/file3 >/dev/null 2>&1 ) &&
-	( yes "d" | git mergetool file11 >/dev/null 2>&1 ) &&
-	( yes "d" | git mergetool file12 >/dev/null 2>&1 ) &&
-	( yes "r" | git mergetool submod >/dev/null 2>&1 ) &&
+	test_must_fail git merge master &&
+	( yes "" | git mergetool file1 ) &&
+	( yes "" | git mergetool file2 ) &&
+	( yes "" | git mergetool "spaced name" ) &&
+	( yes "" | git mergetool both ) &&
+	( yes "" | git mergetool subdir/file3 ) &&
+	( yes "d" | git mergetool file11 ) &&
+	( yes "d" | git mergetool file12 ) &&
+	( yes "r" | git mergetool submod ) &&
 	test "$(printf x | cat file1 -)" = "$(printf "master updated\r\nx")" &&
 	test "$(printf x | cat file2 -)" = "$(printf "master new\r\nx")" &&
 	test "$(printf x | cat subdir/file3 -)" = "$(printf "master new sub\r\nx")" &&
@@ -176,8 +176,8 @@ test_expect_success 'mergetool in subdir' '
 	git submodule update -N &&
 	(
 		cd subdir &&
-		test_must_fail git merge master >/dev/null 2>&1 &&
-		( yes "" | git mergetool file3 >/dev/null 2>&1 ) &&
+		test_must_fail git merge master &&
+		( yes "" | git mergetool file3 ) &&
 		test "$(cat file3)" = "master new sub"
 	)
 '
@@ -188,14 +188,14 @@ test_expect_success 'mergetool on file in parent dir' '
 	git submodule update -N &&
 	(
 		cd subdir &&
-		test_must_fail git merge master >/dev/null 2>&1 &&
-		( yes "" | git mergetool file3 >/dev/null 2>&1 ) &&
-		( yes "" | git mergetool ../file1 >/dev/null 2>&1 ) &&
-		( yes "" | git mergetool ../file2 ../spaced\ name >/dev/null 2>&1 ) &&
-		( yes "" | git mergetool ../both >/dev/null 2>&1 ) &&
-		( yes "d" | git mergetool ../file11 >/dev/null 2>&1 ) &&
-		( yes "d" | git mergetool ../file12 >/dev/null 2>&1 ) &&
-		( yes "l" | git mergetool ../submod >/dev/null 2>&1 ) &&
+		test_must_fail git merge master &&
+		( yes "" | git mergetool file3 ) &&
+		( yes "" | git mergetool ../file1 ) &&
+		( yes "" | git mergetool ../file2 ../spaced\ name ) &&
+		( yes "" | git mergetool ../both ) &&
+		( yes "d" | git mergetool ../file11 ) &&
+		( yes "d" | git mergetool ../file12 ) &&
+		( yes "l" | git mergetool ../submod ) &&
 		test "$(cat ../file1)" = "master updated" &&
 		test "$(cat ../file2)" = "master new" &&
 		test "$(cat ../submod/bar)" = "branch1 submodule" &&
@@ -209,9 +209,9 @@ test_expect_success 'mergetool skips autoresolved' '
 	git submodule update -N &&
 	test_must_fail git merge master &&
 	test -n "$(git ls-files -u)" &&
-	( yes "d" | git mergetool file11 >/dev/null 2>&1 ) &&
-	( yes "d" | git mergetool file12 >/dev/null 2>&1 ) &&
-	( yes "l" | git mergetool submod >/dev/null 2>&1 ) &&
+	( yes "d" | git mergetool file11 ) &&
+	( yes "d" | git mergetool file12 ) &&
+	( yes "l" | git mergetool submod ) &&
 	output="$(git mergetool --no-prompt)" &&
 	test "$output" = "No files need merging"
 '
@@ -259,9 +259,9 @@ test_expect_success 'mergetool skips resolved paths when rerere is active' '
 	rm -rf .git/rr-cache &&
 	git checkout -b test$test_count branch1 &&
 	git submodule update -N &&
-	test_must_fail git merge master >/dev/null 2>&1 &&
-	( yes "l" | git mergetool --no-prompt submod >/dev/null 2>&1 ) &&
-	( yes "d" "d" | git mergetool --no-prompt >/dev/null 2>&1 ) &&
+	test_must_fail git merge master &&
+	( yes "l" | git mergetool --no-prompt submod ) &&
+	( yes "d" "d" | git mergetool --no-prompt ) &&
 	git submodule update -N &&
 	output="$(yes "n" | git mergetool --no-prompt)" &&
 	test "$output" = "No files need merging"
@@ -369,9 +369,9 @@ test_expect_success 'deleted vs modified submodule' '
 	git checkout -b test$test_count.a test$test_count &&
 	test_must_fail git merge master &&
 	test -n "$(git ls-files -u)" &&
-	( yes "" | git mergetool file1 file2 spaced\ name subdir/file3 >/dev/null 2>&1 ) &&
-	( yes "" | git mergetool both >/dev/null 2>&1 ) &&
-	( yes "d" | git mergetool file11 file12 >/dev/null 2>&1 ) &&
+	( yes "" | git mergetool file1 file2 spaced\ name subdir/file3 ) &&
+	( yes "" | git mergetool both ) &&
+	( yes "d" | git mergetool file11 file12 ) &&
 	( yes "r" | git mergetool submod ) &&
 	rmdir submod && mv submod-movedaside submod &&
 	test "$(cat submod/bar)" = "branch1 submodule" &&
@@ -386,9 +386,9 @@ test_expect_success 'deleted vs modified submodule' '
 	git submodule update -N &&
 	test_must_fail git merge master &&
 	test -n "$(git ls-files -u)" &&
-	( yes "" | git mergetool file1 file2 spaced\ name subdir/file3 >/dev/null 2>&1 ) &&
-	( yes "" | git mergetool both >/dev/null 2>&1 ) &&
-	( yes "d" | git mergetool file11 file12 >/dev/null 2>&1 ) &&
+	( yes "" | git mergetool file1 file2 spaced\ name subdir/file3 ) &&
+	( yes "" | git mergetool both ) &&
+	( yes "d" | git mergetool file11 file12 ) &&
 	( yes "l" | git mergetool submod ) &&
 	test ! -e submod &&
 	output="$(git mergetool --no-prompt)" &&
@@ -400,9 +400,9 @@ test_expect_success 'deleted vs modified submodule' '
 	git submodule update -N &&
 	test_must_fail git merge test$test_count &&
 	test -n "$(git ls-files -u)" &&
-	( yes "" | git mergetool file1 file2 spaced\ name subdir/file3 >/dev/null 2>&1 ) &&
-	( yes "" | git mergetool both >/dev/null 2>&1 ) &&
-	( yes "d" | git mergetool file11 file12 >/dev/null 2>&1 ) &&
+	( yes "" | git mergetool file1 file2 spaced\ name subdir/file3 ) &&
+	( yes "" | git mergetool both ) &&
+	( yes "d" | git mergetool file11 file12 ) &&
 	( yes "r" | git mergetool submod ) &&
 	test ! -e submod &&
 	test -d submod.orig &&
@@ -416,9 +416,9 @@ test_expect_success 'deleted vs modified submodule' '
 	git submodule update -N &&
 	test_must_fail git merge test$test_count &&
 	test -n "$(git ls-files -u)" &&
-	( yes "" | git mergetool file1 file2 spaced\ name subdir/file3 >/dev/null 2>&1 ) &&
-	( yes "" | git mergetool both >/dev/null 2>&1 ) &&
-	( yes "d" | git mergetool file11 file12 >/dev/null 2>&1 ) &&
+	( yes "" | git mergetool file1 file2 spaced\ name subdir/file3 ) &&
+	( yes "" | git mergetool both ) &&
+	( yes "d" | git mergetool file11 file12 ) &&
 	( yes "l" | git mergetool submod ) &&
 	test "$(cat submod/bar)" = "master submodule" &&
 	git submodule update -N &&
@@ -440,9 +440,9 @@ test_expect_success 'file vs modified submodule' '
 	git checkout -b test$test_count.a branch1 &&
 	test_must_fail git merge master &&
 	test -n "$(git ls-files -u)" &&
-	( yes "" | git mergetool file1 file2 spaced\ name subdir/file3 >/dev/null 2>&1 ) &&
-	( yes "" | git mergetool both >/dev/null 2>&1 ) &&
-	( yes "d" | git mergetool file11 file12 >/dev/null 2>&1 ) &&
+	( yes "" | git mergetool file1 file2 spaced\ name subdir/file3 ) &&
+	( yes "" | git mergetool both ) &&
+	( yes "d" | git mergetool file11 file12 ) &&
 	( yes "r" | git mergetool submod ) &&
 	rmdir submod && mv submod-movedaside submod &&
 	test "$(cat submod/bar)" = "branch1 submodule" &&
@@ -456,9 +456,9 @@ test_expect_success 'file vs modified submodule' '
 	git checkout -b test$test_count.b test$test_count &&
 	test_must_fail git merge master &&
 	test -n "$(git ls-files -u)" &&
-	( yes "" | git mergetool file1 file2 spaced\ name subdir/file3 >/dev/null 2>&1 ) &&
-	( yes "" | git mergetool both >/dev/null 2>&1 ) &&
-	( yes "d" | git mergetool file11 file12 >/dev/null 2>&1 ) &&
+	( yes "" | git mergetool file1 file2 spaced\ name subdir/file3 ) &&
+	( yes "" | git mergetool both ) &&
+	( yes "d" | git mergetool file11 file12 ) &&
 	( yes "l" | git mergetool submod ) &&
 	git submodule update -N &&
 	test "$(cat submod)" = "not a submodule" &&
@@ -472,9 +472,9 @@ test_expect_success 'file vs modified submodule' '
 	git submodule update -N &&
 	test_must_fail git merge test$test_count &&
 	test -n "$(git ls-files -u)" &&
-	( yes "" | git mergetool file1 file2 spaced\ name subdir/file3 >/dev/null 2>&1 ) &&
-	( yes "" | git mergetool both >/dev/null 2>&1 ) &&
-	( yes "d" | git mergetool file11 file12 >/dev/null 2>&1 ) &&
+	( yes "" | git mergetool file1 file2 spaced\ name subdir/file3 ) &&
+	( yes "" | git mergetool both ) &&
+	( yes "d" | git mergetool file11 file12 ) &&
 	( yes "r" | git mergetool submod ) &&
 	test -d submod.orig &&
 	git submodule update -N &&
@@ -488,9 +488,9 @@ test_expect_success 'file vs modified submodule' '
 	git submodule update -N &&
 	test_must_fail git merge test$test_count &&
 	test -n "$(git ls-files -u)" &&
-	( yes "" | git mergetool file1 file2 spaced\ name subdir/file3 >/dev/null 2>&1 ) &&
-	( yes "" | git mergetool both>/dev/null 2>&1 ) &&
-	( yes "d" | git mergetool file11 file12 >/dev/null 2>&1 ) &&
+	( yes "" | git mergetool file1 file2 spaced\ name subdir/file3 ) &&
+	( yes "" | git mergetool both ) &&
+	( yes "d" | git mergetool file11 file12 ) &&
 	( yes "l" | git mergetool submod ) &&
 	test "$(cat submod/bar)" = "master submodule" &&
 	git submodule update -N &&
@@ -543,7 +543,7 @@ test_expect_success 'submodule in subdirectory' '
 	git add subdir/subdir_module &&
 	git commit -m "change submodule in subdirectory on test$test_count.b" &&
 
-	test_must_fail git merge test$test_count.a >/dev/null 2>&1 &&
+	test_must_fail git merge test$test_count.a &&
 	(
 		cd subdir &&
 		( yes "l" | git mergetool subdir_module )
@@ -554,7 +554,7 @@ test_expect_success 'submodule in subdirectory' '
 	git reset --hard &&
 	git submodule update -N &&
 
-	test_must_fail git merge test$test_count.a >/dev/null 2>&1 &&
+	test_must_fail git merge test$test_count.a &&
 	( yes "r" | git mergetool subdir/subdir_module ) &&
 	test "$(cat subdir/subdir_module/file15)" = "test$test_count.b" &&
 	git submodule update -N &&
@@ -641,7 +641,7 @@ test_expect_success 'filenames seen by tools start with ./' '
 	test_config mergetool.myecho.trustExitCode true &&
 	test_must_fail git merge master &&
 	git mergetool --no-prompt --tool myecho -- both >actual &&
-	grep ^\./both_LOCAL_ actual >/dev/null
+	grep ^\./both_LOCAL_ actual
 '
 
 test_lazy_prereq MKTEMP '
@@ -658,8 +658,8 @@ test_expect_success MKTEMP 'temporary filenames are used with mergetool.writeToT
 	test_config mergetool.myecho.trustExitCode true &&
 	test_must_fail git merge master &&
 	git mergetool --no-prompt --tool myecho -- both >actual &&
-	! grep ^\./both_LOCAL_ actual >/dev/null &&
-	grep /both_LOCAL_ actual >/dev/null
+	! grep ^\./both_LOCAL_ actual &&
+	grep /both_LOCAL_ actual
 '
 
 test_expect_success 'diff.orderFile configuration is honored' '
-- 
2.21.0.1000.g11cd861522


^ permalink raw reply related	[flat|nested] 48+ messages in thread

* [PATCH v4 2/6] t7610: add mergetool --gui tests
  2019-04-25  9:54     ` [PATCH v4 0/6] difftool and mergetool improvements Denton Liu
  2019-04-25  9:54       ` [PATCH v4 1/6] t7610: unsuppress output Denton Liu
@ 2019-04-25  9:54       ` Denton Liu
  2019-04-25  9:54       ` [PATCH v4 3/6] mergetool: use get_merge_tool function Denton Liu
                         ` (4 subsequent siblings)
  6 siblings, 0 replies; 48+ messages in thread
From: Denton Liu @ 2019-04-25  9:54 UTC (permalink / raw)
  To: Git Mailing List
  Cc: Johannes Schindelin, David Aguilar, Jeff Hostetler,
	Eric Sunshine, Junio C Hamano

In 063f2bdbf7 (mergetool: accept -g/--[no-]gui as arguments,
2018-10-24), mergetool was taught the --gui option but no tests were
added to ensure that it was working properly. Add a test to ensure that
it works.

Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
 t/t7610-mergetool.sh | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/t/t7610-mergetool.sh b/t/t7610-mergetool.sh
index 69711487dd..dad607e186 100755
--- a/t/t7610-mergetool.sh
+++ b/t/t7610-mergetool.sh
@@ -145,6 +145,28 @@ test_expect_success 'custom mergetool' '
 	git commit -m "branch1 resolved with mergetool"
 '
 
+test_expect_success 'gui mergetool' '
+	test_config merge.guitool myguitool &&
+	test_config mergetool.myguitool.cmd "(printf \"gui \" && cat \"\$REMOTE\") >\"\$MERGED\"" &&
+	test_config mergetool.myguitool.trustExitCode true &&
+	test_when_finished "git reset --hard" &&
+	git checkout -b test$test_count branch1 &&
+	git submodule update -N &&
+	test_must_fail git merge master &&
+	( yes "" | git mergetool --gui both ) &&
+	( yes "" | git mergetool -g file1 file1 ) &&
+	( yes "" | git mergetool --gui file2 "spaced name" ) &&
+	( yes "" | git mergetool --gui subdir/file3 ) &&
+	( yes "d" | git mergetool --gui file11 ) &&
+	( yes "d" | git mergetool --gui file12 ) &&
+	( yes "l" | git mergetool --gui submod ) &&
+	test "$(cat file1)" = "gui master updated" &&
+	test "$(cat file2)" = "gui master new" &&
+	test "$(cat subdir/file3)" = "gui master new sub" &&
+	test "$(cat submod/bar)" = "branch1 submodule" &&
+	git commit -m "branch1 resolved with mergetool"
+'
+
 test_expect_success 'mergetool crlf' '
 	test_when_finished "git reset --hard" &&
 	# This test_config line must go after the above reset line so that
-- 
2.21.0.1000.g11cd861522


^ permalink raw reply related	[flat|nested] 48+ messages in thread

* [PATCH v4 3/6] mergetool: use get_merge_tool function
  2019-04-25  9:54     ` [PATCH v4 0/6] difftool and mergetool improvements Denton Liu
  2019-04-25  9:54       ` [PATCH v4 1/6] t7610: unsuppress output Denton Liu
  2019-04-25  9:54       ` [PATCH v4 2/6] t7610: add mergetool --gui tests Denton Liu
@ 2019-04-25  9:54       ` Denton Liu
  2019-04-28 23:52         ` David Aguilar
  2019-04-25  9:54       ` [PATCH v4 4/6] mergetool: fallback to tool when guitool unavailable Denton Liu
                         ` (3 subsequent siblings)
  6 siblings, 1 reply; 48+ messages in thread
From: Denton Liu @ 2019-04-25  9:54 UTC (permalink / raw)
  To: Git Mailing List
  Cc: Johannes Schindelin, David Aguilar, Jeff Hostetler,
	Eric Sunshine, Junio C Hamano

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(-)

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


^ permalink raw reply related	[flat|nested] 48+ messages in thread

* [PATCH v4 4/6] mergetool: fallback to tool when guitool unavailable
  2019-04-25  9:54     ` [PATCH v4 0/6] difftool and mergetool improvements Denton Liu
                         ` (2 preceding siblings ...)
  2019-04-25  9:54       ` [PATCH v4 3/6] mergetool: use get_merge_tool function Denton Liu
@ 2019-04-25  9:54       ` Denton Liu
  2019-04-28 23:56         ` David Aguilar
  2019-04-25  9:54       ` [PATCH v4 5/6] difftool: make --gui, --tool and --extcmd mutually exclusive Denton Liu
                         ` (2 subsequent siblings)
  6 siblings, 1 reply; 48+ messages in thread
From: Denton Liu @ 2019-04-25  9:54 UTC (permalink / raw)
  To: Git Mailing List
  Cc: Johannes Schindelin, David Aguilar, Jeff Hostetler,
	Eric Sunshine, Junio C Hamano

In git-difftool, if the tool is called with --gui but `diff.guitool` is
not set, it falls back to `diff.tool`. Make git-mergetool also fallback
from `merge.guitool` to `merge.tool` if the former is undefined.

If git-difftool, when called with `--gui`, were to use
`get_configured_mergetool` in a future patch, it would also get the
fallback behavior in the following precedence:

1. diff.guitool
2. merge.guitool
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.

Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
 Documentation/git-mergetool.txt |  4 +++-
 git-mergetool--lib.sh           | 32 +++++++++++++++++++++++---------
 t/t7610-mergetool.sh            | 19 +++++++++++++++++++
 3 files changed, 45 insertions(+), 10 deletions(-)

diff --git a/Documentation/git-mergetool.txt b/Documentation/git-mergetool.txt
index 0c7975a050..6b14702e78 100644
--- a/Documentation/git-mergetool.txt
+++ b/Documentation/git-mergetool.txt
@@ -83,7 +83,9 @@ success of the resolution after the custom tool has exited.
 --gui::
 	When 'git-mergetool' is invoked with the `-g` or `--gui` option
 	the default merge tool will be read from the configured
-	`merge.guitool` variable instead of `merge.tool`.
+	`merge.guitool` variable instead of `merge.tool`. If
+	`merge.guitool` is not set, we will fallback to the tool
+	configured under `merge.tool`.
 
 --no-gui::
 	This overrides a previous `-g` or `--gui` setting and reads the
diff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh
index 68ff26a0f7..c4b16c5e59 100644
--- a/git-mergetool--lib.sh
+++ b/git-mergetool--lib.sh
@@ -350,20 +350,34 @@ guess_merge_tool () {
 }
 
 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
-		gui_prefix=gui
+		sections="diff $sections"
 	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
 	then
-		merge_tool=$(git config diff.${gui_prefix}tool || git config merge.${gui_prefix}tool)
-	else
-		merge_tool=$(git config merge.${gui_prefix}tool)
+		keys="guitool $keys"
 	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
+		done)
+
 	if test -n "$merge_tool" && ! valid_tool "$merge_tool"
 	then
 		echo >&2 "git config option $TOOL_MODE.${gui_prefix}tool set to unknown tool: $merge_tool"
diff --git a/t/t7610-mergetool.sh b/t/t7610-mergetool.sh
index dad607e186..5b61c10a9c 100755
--- a/t/t7610-mergetool.sh
+++ b/t/t7610-mergetool.sh
@@ -167,6 +167,25 @@ test_expect_success 'gui mergetool' '
 	git commit -m "branch1 resolved with mergetool"
 '
 
+test_expect_success 'gui mergetool without merge.guitool set falls back to merge.tool' '
+	test_when_finished "git reset --hard" &&
+	git checkout -b test$test_count branch1 &&
+	git submodule update -N &&
+	test_must_fail git merge master &&
+	( yes "" | git mergetool --gui both ) &&
+	( yes "" | git mergetool -g file1 file1 ) &&
+	( yes "" | git mergetool --gui file2 "spaced name" ) &&
+	( yes "" | git mergetool --gui subdir/file3 ) &&
+	( yes "d" | git mergetool --gui file11 ) &&
+	( yes "d" | git mergetool --gui file12 ) &&
+	( yes "l" | git mergetool --gui submod ) &&
+	test "$(cat file1)" = "master updated" &&
+	test "$(cat file2)" = "master new" &&
+	test "$(cat subdir/file3)" = "master new sub" &&
+	test "$(cat submod/bar)" = "branch1 submodule" &&
+	git commit -m "branch1 resolved with mergetool"
+'
+
 test_expect_success 'mergetool crlf' '
 	test_when_finished "git reset --hard" &&
 	# This test_config line must go after the above reset line so that
-- 
2.21.0.1000.g11cd861522


^ permalink raw reply related	[flat|nested] 48+ messages in thread

* [PATCH v4 5/6] difftool: make --gui, --tool and --extcmd mutually exclusive
  2019-04-25  9:54     ` [PATCH v4 0/6] difftool and mergetool improvements Denton Liu
                         ` (3 preceding siblings ...)
  2019-04-25  9:54       ` [PATCH v4 4/6] mergetool: fallback to tool when guitool unavailable Denton Liu
@ 2019-04-25  9:54       ` Denton Liu
  2019-04-25  9:54       ` [PATCH v4 6/6] difftool: fallback on merge.guitool Denton Liu
  2019-04-29  6:20       ` [PATCH v5 0/7] difftool and mergetool improvements Denton Liu
  6 siblings, 0 replies; 48+ messages in thread
From: Denton Liu @ 2019-04-25  9:54 UTC (permalink / raw)
  To: Git Mailing List
  Cc: Johannes Schindelin, David Aguilar, Jeff Hostetler,
	Eric Sunshine, Junio C Hamano

In git-difftool, these options specify which tool to ultimately run. As
a result, they are logically conflicting. Explicitly disallow these
options from being used together.

Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
 builtin/difftool.c  | 3 +++
 t/t7800-difftool.sh | 8 ++++++++
 2 files changed, 11 insertions(+)

diff --git a/builtin/difftool.c b/builtin/difftool.c
index a3ea60ea71..65bba90338 100644
--- a/builtin/difftool.c
+++ b/builtin/difftool.c
@@ -731,6 +731,9 @@ int cmd_difftool(int argc, const char **argv, const char *prefix)
 	setenv(GIT_DIR_ENVIRONMENT, absolute_path(get_git_dir()), 1);
 	setenv(GIT_WORK_TREE_ENVIRONMENT, absolute_path(get_git_work_tree()), 1);
 
+	if (use_gui_tool + !!difftool_cmd + !!extcmd > 1)
+		die(_("--gui, --tool and --extcmd are mutually exclusive"));
+
 	if (use_gui_tool && diff_gui_tool && *diff_gui_tool)
 		setenv("GIT_DIFF_TOOL", diff_gui_tool, 1);
 	else if (difftool_cmd) {
diff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh
index bb9a7f4ff9..107f31213d 100755
--- a/t/t7800-difftool.sh
+++ b/t/t7800-difftool.sh
@@ -705,4 +705,12 @@ test_expect_success SYMLINKS 'difftool --dir-diff handles modified symlinks' '
 	test_cmp expect actual
 '
 
+test_expect_success 'difftool --gui, --tool and --extcmd are exclusive' '
+	difftool_test_setup &&
+	test_must_fail git difftool --gui --tool=test-tool &&
+	test_must_fail git difftool --gui --extcmd=cat &&
+	test_must_fail git difftool --tool=test-tool --extcmd=cat &&
+	test_must_fail git difftool --gui --tool=test-tool --extcmd=cat
+'
+
 test_done
-- 
2.21.0.1000.g11cd861522


^ permalink raw reply related	[flat|nested] 48+ messages in thread

* [PATCH v4 6/6] difftool: fallback on merge.guitool
  2019-04-25  9:54     ` [PATCH v4 0/6] difftool and mergetool improvements Denton Liu
                         ` (4 preceding siblings ...)
  2019-04-25  9:54       ` [PATCH v4 5/6] difftool: make --gui, --tool and --extcmd mutually exclusive Denton Liu
@ 2019-04-25  9:54       ` Denton Liu
  2019-04-29  6:20       ` [PATCH v5 0/7] difftool and mergetool improvements Denton Liu
  6 siblings, 0 replies; 48+ messages in thread
From: Denton Liu @ 2019-04-25  9:54 UTC (permalink / raw)
  To: Git Mailing List
  Cc: Johannes Schindelin, David Aguilar, Jeff Hostetler,
	Eric Sunshine, Junio C Hamano

In git-difftool.txt, it says

	'git difftool' falls back to 'git mergetool' config variables when the
	difftool equivalents have not been defined.

However, when `diff.guitool` is missing, it doesn't fallback to
anything. Make git-difftool fallback to `merge.guitool` when `diff.guitool` is
missing.

Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
 Documentation/git-difftool.txt |  4 +++-
 builtin/difftool.c             | 10 ++--------
 t/t7800-difftool.sh            | 16 ++++++++++++++++
 3 files changed, 21 insertions(+), 9 deletions(-)

diff --git a/Documentation/git-difftool.txt b/Documentation/git-difftool.txt
index 96c26e6aa8..484c485fd0 100644
--- a/Documentation/git-difftool.txt
+++ b/Documentation/git-difftool.txt
@@ -90,7 +90,9 @@ instead.  `--no-symlinks` is the default on Windows.
 	When 'git-difftool' is invoked with the `-g` or `--gui` option
 	the default diff tool will be read from the configured
 	`diff.guitool` variable instead of `diff.tool`. The `--no-gui`
-	option can be used to override this setting.
+	option can be used to override this setting. If `diff.guitool`
+	is not set, we will fallback in the order of `merge.guitool`,
+	`diff.tool`, `merge.tool` until a tool is found.
 
 --[no-]trust-exit-code::
 	'git-difftool' invokes a diff tool individually on each file.
diff --git a/builtin/difftool.c b/builtin/difftool.c
index 65bba90338..10660639c0 100644
--- a/builtin/difftool.c
+++ b/builtin/difftool.c
@@ -24,7 +24,6 @@
 #include "object-store.h"
 #include "dir.h"
 
-static char *diff_gui_tool;
 static int trust_exit_code;
 
 static const char *const builtin_difftool_usage[] = {
@@ -34,11 +33,6 @@ static const char *const builtin_difftool_usage[] = {
 
 static int difftool_config(const char *var, const char *value, void *cb)
 {
-	if (!strcmp(var, "diff.guitool")) {
-		diff_gui_tool = xstrdup(value);
-		return 0;
-	}
-
 	if (!strcmp(var, "difftool.trustexitcode")) {
 		trust_exit_code = git_config_bool(var, value);
 		return 0;
@@ -734,8 +728,8 @@ int cmd_difftool(int argc, const char **argv, const char *prefix)
 	if (use_gui_tool + !!difftool_cmd + !!extcmd > 1)
 		die(_("--gui, --tool and --extcmd are mutually exclusive"));
 
-	if (use_gui_tool && diff_gui_tool && *diff_gui_tool)
-		setenv("GIT_DIFF_TOOL", diff_gui_tool, 1);
+	if (use_gui_tool)
+		setenv("GIT_MERGETOOL_GUI", "true", 1);
 	else if (difftool_cmd) {
 		if (*difftool_cmd)
 			setenv("GIT_DIFF_TOOL", difftool_cmd, 1);
diff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh
index 107f31213d..ae90701a12 100755
--- a/t/t7800-difftool.sh
+++ b/t/t7800-difftool.sh
@@ -279,11 +279,27 @@ test_expect_success 'difftool + mergetool config variables' '
 	echo branch >expect &&
 	git difftool --no-prompt branch >actual &&
 	test_cmp expect actual &&
+	git difftool --gui --no-prompt branch >actual &&
+	test_cmp expect actual &&
 
 	# set merge.tool to something bogus, diff.tool to test-tool
 	test_config merge.tool bogus-tool &&
 	test_config diff.tool test-tool &&
 	git difftool --no-prompt branch >actual &&
+	test_cmp expect actual &&
+	git difftool --gui --no-prompt branch >actual &&
+	test_cmp expect actual &&
+
+	# set merge.tool, diff.tool to something bogus, merge.guitool to test-tool
+	test_config diff.tool bogus-tool &&
+	test_config merge.guitool test-tool &&
+	git difftool --gui --no-prompt branch >actual &&
+	test_cmp expect actual &&
+
+	# set merge.tool, diff.tool, merge.guitool to something bogus, diff.guitool to test-tool
+	test_config merge.guitool bogus-tool &&
+	test_config diff.guitool test-tool &&
+	git difftool --gui --no-prompt branch >actual &&
 	test_cmp expect actual
 '
 
-- 
2.21.0.1000.g11cd861522


^ permalink raw reply related	[flat|nested] 48+ messages in thread

* Re: [PATCH v4 3/6] mergetool: use get_merge_tool function
  2019-04-25  9:54       ` [PATCH v4 3/6] mergetool: use get_merge_tool function Denton Liu
@ 2019-04-28 23:52         ` David Aguilar
  0 siblings, 0 replies; 48+ messages in thread
From: David Aguilar @ 2019-04-28 23:52 UTC (permalink / raw)
  To: Denton Liu
  Cc: Git Mailing List, Johannes Schindelin, Jeff Hostetler,
	Eric Sunshine, Junio C Hamano

On Thu, Apr 25, 2019 at 02:54:39AM -0700, Denton Liu wrote:
> 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>


The author of git-diffall approached the list some time ago and his
contribution resulted in "git difftool --dir-diff".

These days we would probably encourage users to use the difftool
built-in feature rather than "diffall", but thank you for your
careful consideration against breaking external scripts.

Maybe we can submit a PR against the diffall repo mentioning
the backstory so that new users are pointed to the main tool.


> 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

Tiny nit; I find double-negatives hard to understand.  Maybe rename this
to "is_guessed" and flip the logic below so that we can keep the
variable named as a positive flag?


>  	# 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
>  }

-- 
David

^ permalink raw reply	[flat|nested] 48+ messages in thread

* Re: [PATCH v4 4/6] mergetool: fallback to tool when guitool unavailable
  2019-04-25  9:54       ` [PATCH v4 4/6] mergetool: fallback to tool when guitool unavailable Denton Liu
@ 2019-04-28 23:56         ` David Aguilar
  0 siblings, 0 replies; 48+ messages in thread
From: David Aguilar @ 2019-04-28 23:56 UTC (permalink / raw)
  To: Denton Liu
  Cc: Git Mailing List, Johannes Schindelin, Jeff Hostetler,
	Eric Sunshine, Junio C Hamano

On Thu, Apr 25, 2019 at 02:54:41AM -0700, Denton Liu wrote:
> diff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh
> index 68ff26a0f7..c4b16c5e59 100644
> --- a/git-mergetool--lib.sh
> +++ b/git-mergetool--lib.sh
> @@ -350,20 +350,34 @@ guess_merge_tool () {
>  }
>  
>  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
> -		gui_prefix=gui
> +		sections="diff $sections"
>  	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

This line looks suspect.  How about,

	if test "$is_gui" = true

instead?  This expression could also be lifted out to an "is_gui"
helper function.


>  	then
> -		merge_tool=$(git config diff.${gui_prefix}tool || git config merge.${gui_prefix}tool)
> -	else
> -		merge_tool=$(git config merge.${gui_prefix}tool)
> +		keys="guitool $keys"
>  	fi
> +
> +	merge_tool=$(
> +		IFS=' '
> +		for key in $keys
> +		do
> +			for section in $sections
> +			do
> +				if selected=$(git config $section.$key)

Would it be simpler to split this conditional into two lines?

	selected=$(git config ...)
	if test -n "$selected"
	then
		...
	fi

Yes, it stops looking at the exit code, but it instead focuses on the
result, which is slightly more bulletproof against a funky user
configuration.


Regarding the two loops above, what would it look like if we
unrolled the logic and just spelled out the keys up front that it's a
little easier to follow?

I agree it is nicer from an implementation sense to use loops,
but we really shouldn't be planning to extend to more permutations in
the future beyond the diff/merge duality, so being explicit and spelling
out each config lookup permutation is simpler to understand since we
only have 4 states.  We should be discouraged from adding any more ;-)

Something like,

	keys=
	if merge_mode
	then
		if gui_mode  # probably worth adding this function
		then
			keys="merge.guitool merge.tool"
		else
			keys="merge.tool"
		fi
	else
		if gui_mode
		then
			keys="diff.guitool merge.guitool diff.tool merge.tool"
		else
			keys="diff.tool merge.tool"
		fi
	fi

.. and then just have a single loop over $keys.
-- 
David

^ permalink raw reply	[flat|nested] 48+ messages in thread

* [PATCH v5 0/7] difftool and mergetool improvements
  2019-04-25  9:54     ` [PATCH v4 0/6] difftool and mergetool improvements Denton Liu
                         ` (5 preceding siblings ...)
  2019-04-25  9:54       ` [PATCH v4 6/6] difftool: fallback on merge.guitool Denton Liu
@ 2019-04-29  6:20       ` Denton Liu
  2019-04-29  6:21         ` [PATCH v5 1/7] t7610: unsuppress output Denton Liu
                           ` (6 more replies)
  6 siblings, 7 replies; 48+ messages in thread
From: Denton Liu @ 2019-04-29  6:20 UTC (permalink / raw)
  To: Git Mailing List
  Cc: Johannes Schindelin, David Aguilar, Jeff Hostetler,
	Eric Sunshine, Junio C Hamano


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@gmail.com>
     
    @@ -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


^ permalink raw reply	[flat|nested] 48+ messages in thread

* [PATCH v5 1/7] t7610: unsuppress output
  2019-04-29  6:20       ` [PATCH v5 0/7] difftool and mergetool improvements Denton Liu
@ 2019-04-29  6:21         ` Denton Liu
  2019-04-29  6:21         ` [PATCH v5 2/7] t7610: add mergetool --gui tests Denton Liu
                           ` (5 subsequent siblings)
  6 siblings, 0 replies; 48+ messages in thread
From: Denton Liu @ 2019-04-29  6:21 UTC (permalink / raw)
  To: Git Mailing List
  Cc: Johannes Schindelin, David Aguilar, Jeff Hostetler,
	Eric Sunshine, Junio C Hamano

The output for commands used to be suppressed by redirecting both stdout
and stderr to /dev/null. However, this should not happen since the
output is useful for debugging and, without the "-v" flag, test scripts
don't output anyway.

Unsuppress the output by removing the redirections to /dev/null.

Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
 t/t7610-mergetool.sh | 122 +++++++++++++++++++++----------------------
 1 file changed, 61 insertions(+), 61 deletions(-)

diff --git a/t/t7610-mergetool.sh b/t/t7610-mergetool.sh
index a9fb971615..69711487dd 100755
--- a/t/t7610-mergetool.sh
+++ b/t/t7610-mergetool.sh
@@ -130,14 +130,14 @@ test_expect_success 'custom mergetool' '
 	test_when_finished "git reset --hard" &&
 	git checkout -b test$test_count branch1 &&
 	git submodule update -N &&
-	test_must_fail git merge master >/dev/null 2>&1 &&
-	( yes "" | git mergetool both >/dev/null 2>&1 ) &&
+	test_must_fail git merge master &&
+	( yes "" | git mergetool both ) &&
 	( yes "" | git mergetool file1 file1 ) &&
-	( yes "" | git mergetool file2 "spaced name" >/dev/null 2>&1 ) &&
-	( yes "" | git mergetool subdir/file3 >/dev/null 2>&1 ) &&
-	( yes "d" | git mergetool file11 >/dev/null 2>&1 ) &&
-	( yes "d" | git mergetool file12 >/dev/null 2>&1 ) &&
-	( yes "l" | git mergetool submod >/dev/null 2>&1 ) &&
+	( yes "" | git mergetool file2 "spaced name" ) &&
+	( yes "" | git mergetool subdir/file3 ) &&
+	( yes "d" | git mergetool file11 ) &&
+	( yes "d" | git mergetool file12 ) &&
+	( yes "l" | git mergetool submod ) &&
 	test "$(cat file1)" = "master updated" &&
 	test "$(cat file2)" = "master new" &&
 	test "$(cat subdir/file3)" = "master new sub" &&
@@ -153,15 +153,15 @@ test_expect_success 'mergetool crlf' '
 	# test_when_finished is LIFO.)
 	test_config core.autocrlf true &&
 	git checkout -b test$test_count branch1 &&
-	test_must_fail git merge master >/dev/null 2>&1 &&
-	( yes "" | git mergetool file1 >/dev/null 2>&1 ) &&
-	( yes "" | git mergetool file2 >/dev/null 2>&1 ) &&
-	( yes "" | git mergetool "spaced name" >/dev/null 2>&1 ) &&
-	( yes "" | git mergetool both >/dev/null 2>&1 ) &&
-	( yes "" | git mergetool subdir/file3 >/dev/null 2>&1 ) &&
-	( yes "d" | git mergetool file11 >/dev/null 2>&1 ) &&
-	( yes "d" | git mergetool file12 >/dev/null 2>&1 ) &&
-	( yes "r" | git mergetool submod >/dev/null 2>&1 ) &&
+	test_must_fail git merge master &&
+	( yes "" | git mergetool file1 ) &&
+	( yes "" | git mergetool file2 ) &&
+	( yes "" | git mergetool "spaced name" ) &&
+	( yes "" | git mergetool both ) &&
+	( yes "" | git mergetool subdir/file3 ) &&
+	( yes "d" | git mergetool file11 ) &&
+	( yes "d" | git mergetool file12 ) &&
+	( yes "r" | git mergetool submod ) &&
 	test "$(printf x | cat file1 -)" = "$(printf "master updated\r\nx")" &&
 	test "$(printf x | cat file2 -)" = "$(printf "master new\r\nx")" &&
 	test "$(printf x | cat subdir/file3 -)" = "$(printf "master new sub\r\nx")" &&
@@ -176,8 +176,8 @@ test_expect_success 'mergetool in subdir' '
 	git submodule update -N &&
 	(
 		cd subdir &&
-		test_must_fail git merge master >/dev/null 2>&1 &&
-		( yes "" | git mergetool file3 >/dev/null 2>&1 ) &&
+		test_must_fail git merge master &&
+		( yes "" | git mergetool file3 ) &&
 		test "$(cat file3)" = "master new sub"
 	)
 '
@@ -188,14 +188,14 @@ test_expect_success 'mergetool on file in parent dir' '
 	git submodule update -N &&
 	(
 		cd subdir &&
-		test_must_fail git merge master >/dev/null 2>&1 &&
-		( yes "" | git mergetool file3 >/dev/null 2>&1 ) &&
-		( yes "" | git mergetool ../file1 >/dev/null 2>&1 ) &&
-		( yes "" | git mergetool ../file2 ../spaced\ name >/dev/null 2>&1 ) &&
-		( yes "" | git mergetool ../both >/dev/null 2>&1 ) &&
-		( yes "d" | git mergetool ../file11 >/dev/null 2>&1 ) &&
-		( yes "d" | git mergetool ../file12 >/dev/null 2>&1 ) &&
-		( yes "l" | git mergetool ../submod >/dev/null 2>&1 ) &&
+		test_must_fail git merge master &&
+		( yes "" | git mergetool file3 ) &&
+		( yes "" | git mergetool ../file1 ) &&
+		( yes "" | git mergetool ../file2 ../spaced\ name ) &&
+		( yes "" | git mergetool ../both ) &&
+		( yes "d" | git mergetool ../file11 ) &&
+		( yes "d" | git mergetool ../file12 ) &&
+		( yes "l" | git mergetool ../submod ) &&
 		test "$(cat ../file1)" = "master updated" &&
 		test "$(cat ../file2)" = "master new" &&
 		test "$(cat ../submod/bar)" = "branch1 submodule" &&
@@ -209,9 +209,9 @@ test_expect_success 'mergetool skips autoresolved' '
 	git submodule update -N &&
 	test_must_fail git merge master &&
 	test -n "$(git ls-files -u)" &&
-	( yes "d" | git mergetool file11 >/dev/null 2>&1 ) &&
-	( yes "d" | git mergetool file12 >/dev/null 2>&1 ) &&
-	( yes "l" | git mergetool submod >/dev/null 2>&1 ) &&
+	( yes "d" | git mergetool file11 ) &&
+	( yes "d" | git mergetool file12 ) &&
+	( yes "l" | git mergetool submod ) &&
 	output="$(git mergetool --no-prompt)" &&
 	test "$output" = "No files need merging"
 '
@@ -259,9 +259,9 @@ test_expect_success 'mergetool skips resolved paths when rerere is active' '
 	rm -rf .git/rr-cache &&
 	git checkout -b test$test_count branch1 &&
 	git submodule update -N &&
-	test_must_fail git merge master >/dev/null 2>&1 &&
-	( yes "l" | git mergetool --no-prompt submod >/dev/null 2>&1 ) &&
-	( yes "d" "d" | git mergetool --no-prompt >/dev/null 2>&1 ) &&
+	test_must_fail git merge master &&
+	( yes "l" | git mergetool --no-prompt submod ) &&
+	( yes "d" "d" | git mergetool --no-prompt ) &&
 	git submodule update -N &&
 	output="$(yes "n" | git mergetool --no-prompt)" &&
 	test "$output" = "No files need merging"
@@ -369,9 +369,9 @@ test_expect_success 'deleted vs modified submodule' '
 	git checkout -b test$test_count.a test$test_count &&
 	test_must_fail git merge master &&
 	test -n "$(git ls-files -u)" &&
-	( yes "" | git mergetool file1 file2 spaced\ name subdir/file3 >/dev/null 2>&1 ) &&
-	( yes "" | git mergetool both >/dev/null 2>&1 ) &&
-	( yes "d" | git mergetool file11 file12 >/dev/null 2>&1 ) &&
+	( yes "" | git mergetool file1 file2 spaced\ name subdir/file3 ) &&
+	( yes "" | git mergetool both ) &&
+	( yes "d" | git mergetool file11 file12 ) &&
 	( yes "r" | git mergetool submod ) &&
 	rmdir submod && mv submod-movedaside submod &&
 	test "$(cat submod/bar)" = "branch1 submodule" &&
@@ -386,9 +386,9 @@ test_expect_success 'deleted vs modified submodule' '
 	git submodule update -N &&
 	test_must_fail git merge master &&
 	test -n "$(git ls-files -u)" &&
-	( yes "" | git mergetool file1 file2 spaced\ name subdir/file3 >/dev/null 2>&1 ) &&
-	( yes "" | git mergetool both >/dev/null 2>&1 ) &&
-	( yes "d" | git mergetool file11 file12 >/dev/null 2>&1 ) &&
+	( yes "" | git mergetool file1 file2 spaced\ name subdir/file3 ) &&
+	( yes "" | git mergetool both ) &&
+	( yes "d" | git mergetool file11 file12 ) &&
 	( yes "l" | git mergetool submod ) &&
 	test ! -e submod &&
 	output="$(git mergetool --no-prompt)" &&
@@ -400,9 +400,9 @@ test_expect_success 'deleted vs modified submodule' '
 	git submodule update -N &&
 	test_must_fail git merge test$test_count &&
 	test -n "$(git ls-files -u)" &&
-	( yes "" | git mergetool file1 file2 spaced\ name subdir/file3 >/dev/null 2>&1 ) &&
-	( yes "" | git mergetool both >/dev/null 2>&1 ) &&
-	( yes "d" | git mergetool file11 file12 >/dev/null 2>&1 ) &&
+	( yes "" | git mergetool file1 file2 spaced\ name subdir/file3 ) &&
+	( yes "" | git mergetool both ) &&
+	( yes "d" | git mergetool file11 file12 ) &&
 	( yes "r" | git mergetool submod ) &&
 	test ! -e submod &&
 	test -d submod.orig &&
@@ -416,9 +416,9 @@ test_expect_success 'deleted vs modified submodule' '
 	git submodule update -N &&
 	test_must_fail git merge test$test_count &&
 	test -n "$(git ls-files -u)" &&
-	( yes "" | git mergetool file1 file2 spaced\ name subdir/file3 >/dev/null 2>&1 ) &&
-	( yes "" | git mergetool both >/dev/null 2>&1 ) &&
-	( yes "d" | git mergetool file11 file12 >/dev/null 2>&1 ) &&
+	( yes "" | git mergetool file1 file2 spaced\ name subdir/file3 ) &&
+	( yes "" | git mergetool both ) &&
+	( yes "d" | git mergetool file11 file12 ) &&
 	( yes "l" | git mergetool submod ) &&
 	test "$(cat submod/bar)" = "master submodule" &&
 	git submodule update -N &&
@@ -440,9 +440,9 @@ test_expect_success 'file vs modified submodule' '
 	git checkout -b test$test_count.a branch1 &&
 	test_must_fail git merge master &&
 	test -n "$(git ls-files -u)" &&
-	( yes "" | git mergetool file1 file2 spaced\ name subdir/file3 >/dev/null 2>&1 ) &&
-	( yes "" | git mergetool both >/dev/null 2>&1 ) &&
-	( yes "d" | git mergetool file11 file12 >/dev/null 2>&1 ) &&
+	( yes "" | git mergetool file1 file2 spaced\ name subdir/file3 ) &&
+	( yes "" | git mergetool both ) &&
+	( yes "d" | git mergetool file11 file12 ) &&
 	( yes "r" | git mergetool submod ) &&
 	rmdir submod && mv submod-movedaside submod &&
 	test "$(cat submod/bar)" = "branch1 submodule" &&
@@ -456,9 +456,9 @@ test_expect_success 'file vs modified submodule' '
 	git checkout -b test$test_count.b test$test_count &&
 	test_must_fail git merge master &&
 	test -n "$(git ls-files -u)" &&
-	( yes "" | git mergetool file1 file2 spaced\ name subdir/file3 >/dev/null 2>&1 ) &&
-	( yes "" | git mergetool both >/dev/null 2>&1 ) &&
-	( yes "d" | git mergetool file11 file12 >/dev/null 2>&1 ) &&
+	( yes "" | git mergetool file1 file2 spaced\ name subdir/file3 ) &&
+	( yes "" | git mergetool both ) &&
+	( yes "d" | git mergetool file11 file12 ) &&
 	( yes "l" | git mergetool submod ) &&
 	git submodule update -N &&
 	test "$(cat submod)" = "not a submodule" &&
@@ -472,9 +472,9 @@ test_expect_success 'file vs modified submodule' '
 	git submodule update -N &&
 	test_must_fail git merge test$test_count &&
 	test -n "$(git ls-files -u)" &&
-	( yes "" | git mergetool file1 file2 spaced\ name subdir/file3 >/dev/null 2>&1 ) &&
-	( yes "" | git mergetool both >/dev/null 2>&1 ) &&
-	( yes "d" | git mergetool file11 file12 >/dev/null 2>&1 ) &&
+	( yes "" | git mergetool file1 file2 spaced\ name subdir/file3 ) &&
+	( yes "" | git mergetool both ) &&
+	( yes "d" | git mergetool file11 file12 ) &&
 	( yes "r" | git mergetool submod ) &&
 	test -d submod.orig &&
 	git submodule update -N &&
@@ -488,9 +488,9 @@ test_expect_success 'file vs modified submodule' '
 	git submodule update -N &&
 	test_must_fail git merge test$test_count &&
 	test -n "$(git ls-files -u)" &&
-	( yes "" | git mergetool file1 file2 spaced\ name subdir/file3 >/dev/null 2>&1 ) &&
-	( yes "" | git mergetool both>/dev/null 2>&1 ) &&
-	( yes "d" | git mergetool file11 file12 >/dev/null 2>&1 ) &&
+	( yes "" | git mergetool file1 file2 spaced\ name subdir/file3 ) &&
+	( yes "" | git mergetool both ) &&
+	( yes "d" | git mergetool file11 file12 ) &&
 	( yes "l" | git mergetool submod ) &&
 	test "$(cat submod/bar)" = "master submodule" &&
 	git submodule update -N &&
@@ -543,7 +543,7 @@ test_expect_success 'submodule in subdirectory' '
 	git add subdir/subdir_module &&
 	git commit -m "change submodule in subdirectory on test$test_count.b" &&
 
-	test_must_fail git merge test$test_count.a >/dev/null 2>&1 &&
+	test_must_fail git merge test$test_count.a &&
 	(
 		cd subdir &&
 		( yes "l" | git mergetool subdir_module )
@@ -554,7 +554,7 @@ test_expect_success 'submodule in subdirectory' '
 	git reset --hard &&
 	git submodule update -N &&
 
-	test_must_fail git merge test$test_count.a >/dev/null 2>&1 &&
+	test_must_fail git merge test$test_count.a &&
 	( yes "r" | git mergetool subdir/subdir_module ) &&
 	test "$(cat subdir/subdir_module/file15)" = "test$test_count.b" &&
 	git submodule update -N &&
@@ -641,7 +641,7 @@ test_expect_success 'filenames seen by tools start with ./' '
 	test_config mergetool.myecho.trustExitCode true &&
 	test_must_fail git merge master &&
 	git mergetool --no-prompt --tool myecho -- both >actual &&
-	grep ^\./both_LOCAL_ actual >/dev/null
+	grep ^\./both_LOCAL_ actual
 '
 
 test_lazy_prereq MKTEMP '
@@ -658,8 +658,8 @@ test_expect_success MKTEMP 'temporary filenames are used with mergetool.writeToT
 	test_config mergetool.myecho.trustExitCode true &&
 	test_must_fail git merge master &&
 	git mergetool --no-prompt --tool myecho -- both >actual &&
-	! grep ^\./both_LOCAL_ actual >/dev/null &&
-	grep /both_LOCAL_ actual >/dev/null
+	! grep ^\./both_LOCAL_ actual &&
+	grep /both_LOCAL_ actual
 '
 
 test_expect_success 'diff.orderFile configuration is honored' '
-- 
2.21.0.1033.g0e8cc1100c


^ permalink raw reply related	[flat|nested] 48+ messages in thread

* [PATCH v5 2/7] t7610: add mergetool --gui tests
  2019-04-29  6:20       ` [PATCH v5 0/7] difftool and mergetool improvements Denton Liu
  2019-04-29  6:21         ` [PATCH v5 1/7] t7610: unsuppress output Denton Liu
@ 2019-04-29  6:21         ` Denton Liu
  2019-04-29  6:21         ` [PATCH v5 3/7] mergetool: use get_merge_tool function Denton Liu
                           ` (4 subsequent siblings)
  6 siblings, 0 replies; 48+ messages in thread
From: Denton Liu @ 2019-04-29  6:21 UTC (permalink / raw)
  To: Git Mailing List
  Cc: Johannes Schindelin, David Aguilar, Jeff Hostetler,
	Eric Sunshine, Junio C Hamano

In 063f2bdbf7 (mergetool: accept -g/--[no-]gui as arguments,
2018-10-24), mergetool was taught the --gui option but no tests were
added to ensure that it was working properly. Add a test to ensure that
it works.

Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
 t/t7610-mergetool.sh | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/t/t7610-mergetool.sh b/t/t7610-mergetool.sh
index 69711487dd..dad607e186 100755
--- a/t/t7610-mergetool.sh
+++ b/t/t7610-mergetool.sh
@@ -145,6 +145,28 @@ test_expect_success 'custom mergetool' '
 	git commit -m "branch1 resolved with mergetool"
 '
 
+test_expect_success 'gui mergetool' '
+	test_config merge.guitool myguitool &&
+	test_config mergetool.myguitool.cmd "(printf \"gui \" && cat \"\$REMOTE\") >\"\$MERGED\"" &&
+	test_config mergetool.myguitool.trustExitCode true &&
+	test_when_finished "git reset --hard" &&
+	git checkout -b test$test_count branch1 &&
+	git submodule update -N &&
+	test_must_fail git merge master &&
+	( yes "" | git mergetool --gui both ) &&
+	( yes "" | git mergetool -g file1 file1 ) &&
+	( yes "" | git mergetool --gui file2 "spaced name" ) &&
+	( yes "" | git mergetool --gui subdir/file3 ) &&
+	( yes "d" | git mergetool --gui file11 ) &&
+	( yes "d" | git mergetool --gui file12 ) &&
+	( yes "l" | git mergetool --gui submod ) &&
+	test "$(cat file1)" = "gui master updated" &&
+	test "$(cat file2)" = "gui master new" &&
+	test "$(cat subdir/file3)" = "gui master new sub" &&
+	test "$(cat submod/bar)" = "branch1 submodule" &&
+	git commit -m "branch1 resolved with mergetool"
+'
+
 test_expect_success 'mergetool crlf' '
 	test_when_finished "git reset --hard" &&
 	# This test_config line must go after the above reset line so that
-- 
2.21.0.1033.g0e8cc1100c


^ permalink raw reply related	[flat|nested] 48+ messages in thread

* [PATCH v5 3/7] mergetool: use get_merge_tool function
  2019-04-29  6:20       ` [PATCH v5 0/7] difftool and mergetool improvements Denton Liu
  2019-04-29  6:21         ` [PATCH v5 1/7] t7610: unsuppress output Denton Liu
  2019-04-29  6:21         ` [PATCH v5 2/7] t7610: add mergetool --gui tests Denton Liu
@ 2019-04-29  6:21         ` Denton Liu
  2019-04-29  6:21         ` [PATCH v5 4/7] mergetool--lib: create gui_mode function Denton Liu
                           ` (3 subsequent siblings)
  6 siblings, 0 replies; 48+ messages in thread
From: Denton Liu @ 2019-04-29  6:21 UTC (permalink / raw)
  To: Git Mailing List
  Cc: Johannes Schindelin, David Aguilar, Jeff Hostetler,
	Eric Sunshine, Junio C Hamano

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

[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(-)

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..b928179a2e 100644
--- a/git-mergetool--lib.sh
+++ b/git-mergetool--lib.sh
@@ -403,14 +403,17 @@ get_merge_tool_path () {
 }
 
 get_merge_tool () {
+	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)
 	# 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
+		is_guessed=true
 	fi
 	echo "$merge_tool"
+	test "$is_guessed" = false
 }
 
 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
-- 
2.21.0.1033.g0e8cc1100c


^ permalink raw reply related	[flat|nested] 48+ messages in thread

* [PATCH v5 4/7] mergetool--lib: create gui_mode function
  2019-04-29  6:20       ` [PATCH v5 0/7] difftool and mergetool improvements Denton Liu
                           ` (2 preceding siblings ...)
  2019-04-29  6:21         ` [PATCH v5 3/7] mergetool: use get_merge_tool function Denton Liu
@ 2019-04-29  6:21         ` Denton Liu
  2019-04-29  6:21         ` [PATCH v5 5/7] mergetool: fallback to tool when guitool unavailable Denton Liu
                           ` (2 subsequent siblings)
  6 siblings, 0 replies; 48+ messages in thread
From: Denton Liu @ 2019-04-29  6:21 UTC (permalink / raw)
  To: Git Mailing List
  Cc: Johannes Schindelin, David Aguilar, Jeff Hostetler,
	Eric Sunshine, Junio C Hamano

Before, in `get_configured_merge_tool`, we would test the value of the
first argument directly, which corresponded to whether we were using
guitool. However, since `$GIT_MERGETOOL_GUI` is available as an
environment variable, create the `gui_mode` function which increases the
clarify of functions which use it.

While we're at it, add a space before `()` in function definitions to
fix the style.

Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
 git-mergetool--lib.sh | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh
index b928179a2e..4ca170c8a7 100644
--- a/git-mergetool--lib.sh
+++ b/git-mergetool--lib.sh
@@ -80,14 +80,18 @@ show_tool_names () {
 	}
 }
 
-diff_mode() {
+diff_mode () {
 	test "$TOOL_MODE" = diff
 }
 
-merge_mode() {
+merge_mode () {
 	test "$TOOL_MODE" = merge
 }
 
+gui_mode () {
+	test "$GIT_MERGETOOL_GUI" = true
+}
+
 translate_merge_tool_path () {
 	echo "$1"
 }
@@ -350,8 +354,7 @@ guess_merge_tool () {
 }
 
 get_configured_merge_tool () {
-	# If first argument is true, find the guitool instead
-	if test "$1" = true
+	if gui_mode
 	then
 		gui_prefix=gui
 	fi
@@ -405,7 +408,7 @@ get_merge_tool_path () {
 get_merge_tool () {
 	is_guessed=false
 	# Check if a merge tool has been configured
-	merge_tool=$(get_configured_merge_tool $GIT_MERGETOOL_GUI)
+	merge_tool=$(get_configured_merge_tool)
 	# Try to guess an appropriate merge tool if no tool has been set.
 	if test -z "$merge_tool"
 	then
-- 
2.21.0.1033.g0e8cc1100c


^ permalink raw reply related	[flat|nested] 48+ messages in thread

* [PATCH v5 5/7] mergetool: fallback to tool when guitool unavailable
  2019-04-29  6:20       ` [PATCH v5 0/7] difftool and mergetool improvements Denton Liu
                           ` (3 preceding siblings ...)
  2019-04-29  6:21         ` [PATCH v5 4/7] mergetool--lib: create gui_mode function Denton Liu
@ 2019-04-29  6:21         ` Denton Liu
  2019-04-29  6:21         ` [PATCH v5 6/7] difftool: make --gui, --tool and --extcmd mutually exclusive Denton Liu
  2019-04-29  6:21         ` [PATCH v5 7/7] difftool: fallback on merge.guitool Denton Liu
  6 siblings, 0 replies; 48+ messages in thread
From: Denton Liu @ 2019-04-29  6:21 UTC (permalink / raw)
  To: Git Mailing List
  Cc: Johannes Schindelin, David Aguilar, Jeff Hostetler,
	Eric Sunshine, Junio C Hamano

In git-difftool, if the tool is called with --gui but `diff.guitool` is
not set, it falls back to `diff.tool`. Make git-mergetool also fallback
from `merge.guitool` to `merge.tool` if the former is undefined.

If git-difftool, when called with `--gui`, were to use
`get_configured_mergetool` in a future patch, it would also get the
fallback behavior in the following precedence:

1. diff.guitool
2. merge.guitool
3. diff.tool
4. merge.tool

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 is more
readable for future readers.

Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
 Documentation/git-mergetool.txt |  4 +++-
 git-mergetool--lib.sh           | 35 ++++++++++++++++++++++++---------
 t/t7610-mergetool.sh            | 19 ++++++++++++++++++
 3 files changed, 48 insertions(+), 10 deletions(-)

diff --git a/Documentation/git-mergetool.txt b/Documentation/git-mergetool.txt
index 0c7975a050..6b14702e78 100644
--- a/Documentation/git-mergetool.txt
+++ b/Documentation/git-mergetool.txt
@@ -83,7 +83,9 @@ success of the resolution after the custom tool has exited.
 --gui::
 	When 'git-mergetool' is invoked with the `-g` or `--gui` option
 	the default merge tool will be read from the configured
-	`merge.guitool` variable instead of `merge.tool`.
+	`merge.guitool` variable instead of `merge.tool`. If
+	`merge.guitool` is not set, we will fallback to the tool
+	configured under `merge.tool`.
 
 --no-gui::
 	This overrides a previous `-g` or `--gui` setting and reads the
diff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh
index 4ca170c8a7..696eb49160 100644
--- a/git-mergetool--lib.sh
+++ b/git-mergetool--lib.sh
@@ -354,19 +354,36 @@ guess_merge_tool () {
 }
 
 get_configured_merge_tool () {
-	if gui_mode
-	then
-		gui_prefix=gui
-	fi
-
-	# Diff mode first tries diff.(gui)tool and falls back to merge.(gui)tool.
-	# Merge mode only checks merge.(gui)tool
+	keys=
 	if diff_mode
 	then
-		merge_tool=$(git config diff.${gui_prefix}tool || git config merge.${gui_prefix}tool)
+		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)
+		if gui_mode
+		then
+			keys="merge.guitool merge.tool"
+		else
+			keys="merge.tool"
+		fi
 	fi
+
+	merge_tool=$(
+		IFS=' '
+		for key in $keys
+		do
+			selected=$(git config $key)
+			if test -n "$selected"
+			then
+				echo "$selected"
+				return
+			fi
+		done)
+
 	if test -n "$merge_tool" && ! valid_tool "$merge_tool"
 	then
 		echo >&2 "git config option $TOOL_MODE.${gui_prefix}tool set to unknown tool: $merge_tool"
diff --git a/t/t7610-mergetool.sh b/t/t7610-mergetool.sh
index dad607e186..5b61c10a9c 100755
--- a/t/t7610-mergetool.sh
+++ b/t/t7610-mergetool.sh
@@ -167,6 +167,25 @@ test_expect_success 'gui mergetool' '
 	git commit -m "branch1 resolved with mergetool"
 '
 
+test_expect_success 'gui mergetool without merge.guitool set falls back to merge.tool' '
+	test_when_finished "git reset --hard" &&
+	git checkout -b test$test_count branch1 &&
+	git submodule update -N &&
+	test_must_fail git merge master &&
+	( yes "" | git mergetool --gui both ) &&
+	( yes "" | git mergetool -g file1 file1 ) &&
+	( yes "" | git mergetool --gui file2 "spaced name" ) &&
+	( yes "" | git mergetool --gui subdir/file3 ) &&
+	( yes "d" | git mergetool --gui file11 ) &&
+	( yes "d" | git mergetool --gui file12 ) &&
+	( yes "l" | git mergetool --gui submod ) &&
+	test "$(cat file1)" = "master updated" &&
+	test "$(cat file2)" = "master new" &&
+	test "$(cat subdir/file3)" = "master new sub" &&
+	test "$(cat submod/bar)" = "branch1 submodule" &&
+	git commit -m "branch1 resolved with mergetool"
+'
+
 test_expect_success 'mergetool crlf' '
 	test_when_finished "git reset --hard" &&
 	# This test_config line must go after the above reset line so that
-- 
2.21.0.1033.g0e8cc1100c


^ permalink raw reply related	[flat|nested] 48+ messages in thread

* [PATCH v5 6/7] difftool: make --gui, --tool and --extcmd mutually exclusive
  2019-04-29  6:20       ` [PATCH v5 0/7] difftool and mergetool improvements Denton Liu
                           ` (4 preceding siblings ...)
  2019-04-29  6:21         ` [PATCH v5 5/7] mergetool: fallback to tool when guitool unavailable Denton Liu
@ 2019-04-29  6:21         ` Denton Liu
  2019-04-29  6:21         ` [PATCH v5 7/7] difftool: fallback on merge.guitool Denton Liu
  6 siblings, 0 replies; 48+ messages in thread
From: Denton Liu @ 2019-04-29  6:21 UTC (permalink / raw)
  To: Git Mailing List
  Cc: Johannes Schindelin, David Aguilar, Jeff Hostetler,
	Eric Sunshine, Junio C Hamano

In git-difftool, these options specify which tool to ultimately run. As
a result, they are logically conflicting. Explicitly disallow these
options from being used together.

Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
 builtin/difftool.c  | 3 +++
 t/t7800-difftool.sh | 8 ++++++++
 2 files changed, 11 insertions(+)

diff --git a/builtin/difftool.c b/builtin/difftool.c
index a3ea60ea71..65bba90338 100644
--- a/builtin/difftool.c
+++ b/builtin/difftool.c
@@ -731,6 +731,9 @@ int cmd_difftool(int argc, const char **argv, const char *prefix)
 	setenv(GIT_DIR_ENVIRONMENT, absolute_path(get_git_dir()), 1);
 	setenv(GIT_WORK_TREE_ENVIRONMENT, absolute_path(get_git_work_tree()), 1);
 
+	if (use_gui_tool + !!difftool_cmd + !!extcmd > 1)
+		die(_("--gui, --tool and --extcmd are mutually exclusive"));
+
 	if (use_gui_tool && diff_gui_tool && *diff_gui_tool)
 		setenv("GIT_DIFF_TOOL", diff_gui_tool, 1);
 	else if (difftool_cmd) {
diff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh
index bb9a7f4ff9..fd4a2a93b6 100755
--- a/t/t7800-difftool.sh
+++ b/t/t7800-difftool.sh
@@ -705,4 +705,12 @@ test_expect_success SYMLINKS 'difftool --dir-diff handles modified symlinks' '
 	test_cmp expect actual
 '
 
+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 &&
+	test_must_fail git difftool --tool=test-tool --extcmd=cat &&
+	test_must_fail git difftool --gui --tool=test-tool --extcmd=cat
+'
+
 test_done
-- 
2.21.0.1033.g0e8cc1100c


^ permalink raw reply related	[flat|nested] 48+ messages in thread

* [PATCH v5 7/7] difftool: fallback on merge.guitool
  2019-04-29  6:20       ` [PATCH v5 0/7] difftool and mergetool improvements Denton Liu
                           ` (5 preceding siblings ...)
  2019-04-29  6:21         ` [PATCH v5 6/7] difftool: make --gui, --tool and --extcmd mutually exclusive Denton Liu
@ 2019-04-29  6:21         ` Denton Liu
  6 siblings, 0 replies; 48+ messages in thread
From: Denton Liu @ 2019-04-29  6:21 UTC (permalink / raw)
  To: Git Mailing List
  Cc: Johannes Schindelin, David Aguilar, Jeff Hostetler,
	Eric Sunshine, Junio C Hamano

In git-difftool.txt, it says

	'git difftool' falls back to 'git mergetool' config variables when the
	difftool equivalents have not been defined.

However, when `diff.guitool` is missing, it doesn't fallback to
anything. Make git-difftool fallback to `merge.guitool` when `diff.guitool` is
missing.

Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
 Documentation/git-difftool.txt |  4 +++-
 builtin/difftool.c             | 10 ++--------
 t/t7800-difftool.sh            | 16 ++++++++++++++++
 3 files changed, 21 insertions(+), 9 deletions(-)

diff --git a/Documentation/git-difftool.txt b/Documentation/git-difftool.txt
index 96c26e6aa8..484c485fd0 100644
--- a/Documentation/git-difftool.txt
+++ b/Documentation/git-difftool.txt
@@ -90,7 +90,9 @@ instead.  `--no-symlinks` is the default on Windows.
 	When 'git-difftool' is invoked with the `-g` or `--gui` option
 	the default diff tool will be read from the configured
 	`diff.guitool` variable instead of `diff.tool`. The `--no-gui`
-	option can be used to override this setting.
+	option can be used to override this setting. If `diff.guitool`
+	is not set, we will fallback in the order of `merge.guitool`,
+	`diff.tool`, `merge.tool` until a tool is found.
 
 --[no-]trust-exit-code::
 	'git-difftool' invokes a diff tool individually on each file.
diff --git a/builtin/difftool.c b/builtin/difftool.c
index 65bba90338..10660639c0 100644
--- a/builtin/difftool.c
+++ b/builtin/difftool.c
@@ -24,7 +24,6 @@
 #include "object-store.h"
 #include "dir.h"
 
-static char *diff_gui_tool;
 static int trust_exit_code;
 
 static const char *const builtin_difftool_usage[] = {
@@ -34,11 +33,6 @@ static const char *const builtin_difftool_usage[] = {
 
 static int difftool_config(const char *var, const char *value, void *cb)
 {
-	if (!strcmp(var, "diff.guitool")) {
-		diff_gui_tool = xstrdup(value);
-		return 0;
-	}
-
 	if (!strcmp(var, "difftool.trustexitcode")) {
 		trust_exit_code = git_config_bool(var, value);
 		return 0;
@@ -734,8 +728,8 @@ int cmd_difftool(int argc, const char **argv, const char *prefix)
 	if (use_gui_tool + !!difftool_cmd + !!extcmd > 1)
 		die(_("--gui, --tool and --extcmd are mutually exclusive"));
 
-	if (use_gui_tool && diff_gui_tool && *diff_gui_tool)
-		setenv("GIT_DIFF_TOOL", diff_gui_tool, 1);
+	if (use_gui_tool)
+		setenv("GIT_MERGETOOL_GUI", "true", 1);
 	else if (difftool_cmd) {
 		if (*difftool_cmd)
 			setenv("GIT_DIFF_TOOL", difftool_cmd, 1);
diff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh
index fd4a2a93b6..957ddf5dc6 100755
--- a/t/t7800-difftool.sh
+++ b/t/t7800-difftool.sh
@@ -279,11 +279,27 @@ test_expect_success 'difftool + mergetool config variables' '
 	echo branch >expect &&
 	git difftool --no-prompt branch >actual &&
 	test_cmp expect actual &&
+	git difftool --gui --no-prompt branch >actual &&
+	test_cmp expect actual &&
 
 	# set merge.tool to something bogus, diff.tool to test-tool
 	test_config merge.tool bogus-tool &&
 	test_config diff.tool test-tool &&
 	git difftool --no-prompt branch >actual &&
+	test_cmp expect actual &&
+	git difftool --gui --no-prompt branch >actual &&
+	test_cmp expect actual &&
+
+	# set merge.tool, diff.tool to something bogus, merge.guitool to test-tool
+	test_config diff.tool bogus-tool &&
+	test_config merge.guitool test-tool &&
+	git difftool --gui --no-prompt branch >actual &&
+	test_cmp expect actual &&
+
+	# set merge.tool, diff.tool, merge.guitool to something bogus, diff.guitool to test-tool
+	test_config merge.guitool bogus-tool &&
+	test_config diff.guitool test-tool &&
+	git difftool --gui --no-prompt branch >actual &&
 	test_cmp expect actual
 '
 
-- 
2.21.0.1033.g0e8cc1100c


^ permalink raw reply related	[flat|nested] 48+ messages in thread

end of thread, other threads:[~2019-04-29  6:21 UTC | newest]

Thread overview: 48+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-04-22  5:07 [PATCH 0/5] difftool and mergetool improvements Denton Liu
2019-04-22  5:07 ` [PATCH 1/5] t7610: add mergetool --gui tests Denton Liu
2019-04-22  5:07 ` [PATCH 2/5] mergetool: use get_merge_tool function Denton Liu
2019-04-22  7:07   ` Eric Sunshine
2019-04-22  8:35     ` Denton Liu
2019-04-22  5:07 ` [PATCH 3/5] mergetool: fallback to tool when guitool unavailable Denton Liu
2019-04-22  5:07 ` [PATCH 4/5] difftool: make --gui, --tool and --extcmd exclusive Denton Liu
2019-04-22  7:03   ` Eric Sunshine
2019-04-22  5:07 ` [PATCH 5/5] difftool: fallback on merge.guitool Denton Liu
2019-04-22 18:18   ` Jeff Hostetler
2019-04-22 18:33     ` Denton Liu
2019-04-23  8:53 ` [PATCH v2 0/5] difftool and mergetool improvements Denton Liu
2019-04-23  8:53   ` [PATCH v2 1/5] t7610: add mergetool --gui tests Denton Liu
2019-04-24  7:07     ` Junio C Hamano
2019-04-23  8:54   ` [PATCH v2 2/5] mergetool: use get_merge_tool_guessed function Denton Liu
2019-04-24  7:27     ` Junio C Hamano
2019-04-23  8:54   ` [PATCH v2 3/5] mergetool: fallback to tool when guitool unavailable Denton Liu
2019-04-23  8:54   ` [PATCH v2 4/5] difftool: make --gui, --tool and --extcmd mutually exclusive Denton Liu
2019-04-23  8:54   ` [PATCH v2 5/5] difftool: fallback on merge.guitool Denton Liu
2019-04-24 22:46   ` [PATCH v3 0/6] difftool and mergetool improvements Denton Liu
2019-04-24 22:46     ` [PATCH v3 1/6] t7610: unsuppress output Denton Liu
2019-04-25  2:31       ` Junio C Hamano
2019-04-24 22:46     ` [PATCH v3 2/6] t7610: add mergetool --gui tests Denton Liu
2019-04-24 22:47     ` [PATCH v3 3/6] mergetool: use get_merge_tool function Denton Liu
2019-04-25  2:36       ` Junio C Hamano
2019-04-24 22:47     ` [PATCH v3 4/6] mergetool: fallback to tool when guitool unavailable Denton Liu
2019-04-25  3:02       ` Junio C Hamano
2019-04-25  5:16         ` Denton Liu
2019-04-24 22:47     ` [PATCH v3 5/6] difftool: make --gui, --tool and --extcmd mutually exclusive Denton Liu
2019-04-24 22:47     ` [PATCH v3 6/6] difftool: fallback on merge.guitool Denton Liu
2019-04-25  3:10       ` Junio C Hamano
2019-04-25  9:54     ` [PATCH v4 0/6] difftool and mergetool improvements Denton Liu
2019-04-25  9:54       ` [PATCH v4 1/6] t7610: unsuppress output Denton Liu
2019-04-25  9:54       ` [PATCH v4 2/6] t7610: add mergetool --gui tests Denton Liu
2019-04-25  9:54       ` [PATCH v4 3/6] mergetool: use get_merge_tool function Denton Liu
2019-04-28 23:52         ` David Aguilar
2019-04-25  9:54       ` [PATCH v4 4/6] mergetool: fallback to tool when guitool unavailable Denton Liu
2019-04-28 23:56         ` David Aguilar
2019-04-25  9:54       ` [PATCH v4 5/6] difftool: make --gui, --tool and --extcmd mutually exclusive Denton Liu
2019-04-25  9:54       ` [PATCH v4 6/6] difftool: fallback on merge.guitool Denton Liu
2019-04-29  6:20       ` [PATCH v5 0/7] difftool and mergetool improvements Denton Liu
2019-04-29  6:21         ` [PATCH v5 1/7] t7610: unsuppress output Denton Liu
2019-04-29  6:21         ` [PATCH v5 2/7] t7610: add mergetool --gui tests Denton Liu
2019-04-29  6:21         ` [PATCH v5 3/7] mergetool: use get_merge_tool function Denton Liu
2019-04-29  6:21         ` [PATCH v5 4/7] mergetool--lib: create gui_mode function Denton Liu
2019-04-29  6:21         ` [PATCH v5 5/7] mergetool: fallback to tool when guitool unavailable Denton Liu
2019-04-29  6:21         ` [PATCH v5 6/7] difftool: make --gui, --tool and --extcmd mutually exclusive Denton Liu
2019-04-29  6:21         ` [PATCH v5 7/7] difftool: fallback on merge.guitool Denton Liu

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