threads / patch / 32723

patch, 7 partsgit-mergetool: don't hardcode 'mergetool' in show_tool_help

Subject: [PATCH 3/7] git-mergetool: don't hardcode 'mergetool' in show_tool_help

## tl;dr

30 messages between Jan 25, 2013 and Jan 26, 2013. Diffs are folded; open one to read it.

replies: 29people: 4as markdown or json

David Aguilar· Jan 25, 2013, 09:43 UTC · lore

[PATCH 0/7] mergetool-lib improvements for --tool-help

I ran with John's idea and simplified a few more things so that the --tool-help output shows the correct results.

My final commit is dependent on John's commits but the other two could be picked up independently since they are general improvements.

This does add a few symlinks to the repo for the sake of simplicity; I'm not sure if this presents a problem for folks on other platforms.

The replacement of vim with vimdiff and symlinks simplifies things so that we can later auto-generate the list-of-all-tools for use by Documentation/. This was not previously possible due to the special-case for the vim tools which this series eliminates.

David Aguilar (3):
  mergetools/vim: Remove redundant diff command
  mergetools: Fix difftool/mergetool --tool-help listing for vim
  mergetool--lib: Improve show_tool_help() output
John Keeping (4):
  git-mergetool: move show_tool_help to mergetool--lib
  git-mergetool: remove redundant assignment
  git-mergetool: don't hardcode 'mergetool' in show_tool_help
  git-difftool: use git-mergetool--lib for "--tool-help"
 git-difftool.perl           | 55 +++++----------------------------------
 git-mergetool--lib.sh       | 63 +++++++++++++++++++++++++++++++++++++++------
 git-mergetool.sh            | 37 --------------------------
 mergetools/gvimdiff         |  1 +
 mergetools/gvimdiff2        |  1 +
 mergetools/{vim => vimdiff} | 12 ++-------
 mergetools/vimdiff2         |  1 +
 7 files changed, 67 insertions(+), 103 deletions(-)
 create mode 120000 mergetools/gvimdiff
 create mode 120000 mergetools/gvimdiff2
 rename mergetools/{vim => vimdiff} (68%)
 create mode 120000 mergetools/vimdiff2
-- 
1.8.1.1.367.g22b1720.dirty
David Aguilar· Jan 25, 2013, 09:43 UTC · re: David Aguilar · lore

[PATCH 1/7] git-mergetool: move show_tool_help to mergetool--lib

From: John Keeping <john@keeping.me.uk>

This is the first step in unifying "git difftool --tool-help" and "git mergetool --tool-help".

Signed-off-by: John Keeping <john@keeping.me.uk>
Signed-off-by: David Aguilar <davvid@gmail.com>
---
 git-mergetool--lib.sh | 37 +++++++++++++++++++++++++++++++++++++
 git-mergetool.sh      | 37 -------------------------------------
 2 files changed, 37 insertions(+), 37 deletions(-)
Show changes to 2 files +37 −37

git-mergetool--lib.sh, git-mergetool.sh

diff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh
index f013a03..89a857f 100644
--- a/git-mergetool--lib.sh
+++ b/git-mergetool--lib.sh
@@ -174,6 +174,43 @@ list_merge_tool_candidates () {
 	esac
 }
 
+show_tool_help () {
+	TOOL_MODE=merge
+	list_merge_tool_candidates
+	unavailable= available= LF='
+'
+	for i in $tools
+	do
+		merge_tool_path=$(translate_merge_tool_path "$i")
+		if type "$merge_tool_path" >/dev/null 2>&1
+		then
+			available="$available$i$LF"
+		else
+			unavailable="$unavailable$i$LF"
+		fi
+	done
+	if test -n "$available"
+	then
+		echo "'git mergetool --tool=<tool>' may be set to one of the following:"
+		echo "$available" | sort | sed -e 's/^/	/'
+	else
+		echo "No suitable tool for 'git mergetool --tool=<tool>' found."
+	fi
+	if test -n "$unavailable"
+	then
+		echo
+		echo 'The following tools are valid, but not currently available:'
+		echo "$unavailable" | sort | sed -e 's/^/	/'
+	fi
+	if test -n "$unavailable$available"
+	then
+		echo
+		echo "Some of the tools listed above only work in a windowed"
+		echo "environment. If run in a terminal-only session, they will fail."
+	fi
+	exit 0
+}
+
 guess_merge_tool () {
 	list_merge_tool_candidates
 	echo >&2 "merge tool candidates: $tools"
diff --git a/git-mergetool.sh b/git-mergetool.sh
index c50e18a..c0ee9aa 100755
--- a/git-mergetool.sh
+++ b/git-mergetool.sh
@@ -315,43 +315,6 @@ merge_file () {
 	return 0
 }
 
-show_tool_help () {
-	TOOL_MODE=merge
-	list_merge_tool_candidates
-	unavailable= available= LF='
-'
-	for i in $tools
-	do
-		merge_tool_path=$(translate_merge_tool_path "$i")
-		if type "$merge_tool_path" >/dev/null 2>&1
-		then
-			available="$available$i$LF"
-		else
-			unavailable="$unavailable$i$LF"
-		fi
-	done
-	if test -n "$available"
-	then
-		echo "'git mergetool --tool=<tool>' may be set to one of the following:"
-		echo "$available" | sort | sed -e 's/^/	/'
-	else
-		echo "No suitable tool for 'git mergetool --tool=<tool>' found."
-	fi
-	if test -n "$unavailable"
-	then
-		echo
-		echo 'The following tools are valid, but not currently available:'
-		echo "$unavailable" | sort | sed -e 's/^/	/'
-	fi
-	if test -n "$unavailable$available"
-	then
-		echo
-		echo "Some of the tools listed above only work in a windowed"
-		echo "environment. If run in a terminal-only session, they will fail."
-	fi
-	exit 0
-}
-
 prompt=$(git config --bool mergetool.prompt || echo true)
 
 while test $# != 0

-- 
1.8.1.1.367.g22b1720.dirty
David Aguilar· Jan 25, 2013, 09:43 UTC · re: David Aguilar · lore

[PATCH 2/7] git-mergetool: remove redundant assignment

From: John Keeping <john@keeping.me.uk>

TOOL_MODE is set at the top of git-mergetool.sh so there is no need to set it again in show_tool_help. Removing this lets us re-use show_tool_help in git-difftool.

Signed-off-by: John Keeping <john@keeping.me.uk>
Signed-off-by: David Aguilar <davvid@gmail.com>
---
 git-mergetool--lib.sh | 1 -
 1 file changed, 1 deletion(-)
Show changes to git-mergetool--lib.sh +0 −1
diff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh
index 89a857f..1748315 100644
--- a/git-mergetool--lib.sh
+++ b/git-mergetool--lib.sh
@@ -175,7 +175,6 @@ list_merge_tool_candidates () {
 }
 
 show_tool_help () {
-	TOOL_MODE=merge
 	list_merge_tool_candidates
 	unavailable= available= LF='
 '

-- 
1.8.1.1.367.g22b1720.dirty
David Aguilar· Jan 25, 2013, 09:43 UTC · re: David Aguilar · lore
From: John Keeping <john@keeping.me.uk>

When using show_tool_help from git-difftool we will want it to print "git difftool" not "git mergetool" so use "git ${TOOL_MODE}tool".

Signed-off-by: John Keeping <john@keeping.me.uk>
Signed-off-by: David Aguilar <davvid@gmail.com>
---
 git-mergetool--lib.sh | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)
Show changes to git-mergetool--lib.sh +4 −2
diff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh
index 1748315..4c1e129 100644
--- a/git-mergetool--lib.sh
+++ b/git-mergetool--lib.sh
@@ -188,12 +188,14 @@ show_tool_help () {
 			unavailable="$unavailable$i$LF"
 		fi
 	done
+
+	cmd_name=${TOOL_MODE}tool
 	if test -n "$available"
 	then
-		echo "'git mergetool --tool=<tool>' may be set to one of the following:"
+		echo "'git $cmd_name --tool=<tool>' may be set to one of the following:"
 		echo "$available" | sort | sed -e 's/^/	/'
 	else
-		echo "No suitable tool for 'git mergetool --tool=<tool>' found."
+		echo "No suitable tool for 'git $cmd_name --tool=<tool>' found."
 	fi
 	if test -n "$unavailable"
 	then

-- 
1.8.1.1.367.g22b1720.dirty
David Aguilar· Jan 25, 2013, 09:43 UTC · re: David Aguilar · lore

[PATCH 4/7] git-difftool: use git-mergetool--lib for "--tool-help"

From: John Keeping <john@keeping.me.uk>

The "--tool-help" option to git-difftool currently displays incorrect output since it uses the names of the files in "$GIT_EXEC_PATH/mergetools/" rather than the list of command names in git-mergetool--lib.

Fix this by simply delegating the "--tool-help" argument to the show_tool_help function in git-mergetool--lib.

Signed-off-by: John Keeping <john@keeping.me.uk>
Signed-off-by: David Aguilar <davvid@gmail.com>
---
 git-difftool.perl | 55 +++++++------------------------------------------------
 1 file changed, 7 insertions(+), 48 deletions(-)
Show changes to git-difftool.perl +7 −48
diff --git a/git-difftool.perl b/git-difftool.perl
index edd0493..0a90de4 100755
--- a/git-difftool.perl
+++ b/git-difftool.perl
@@ -59,57 +59,16 @@ sub find_worktree
 	return $worktree;
 }
 
-sub filter_tool_scripts
-{
-	my ($tools) = @_;
-	if (-d $_) {
-		if ($_ ne ".") {
-			# Ignore files in subdirectories
-			$File::Find::prune = 1;
-		}
-	} else {
-		if ((-f $_) && ($_ ne "defaults")) {
-			push(@$tools, $_);
-		}
-	}
-}
-
 sub print_tool_help
 {
-	my ($cmd, @found, @notfound, @tools);
-	my $gitpath = Git::exec_path();
-
-	find(sub { filter_tool_scripts(\@tools) }, "$gitpath/mergetools");
-
-	foreach my $tool (@tools) {
-		$cmd  = "TOOL_MODE=diff";
-		$cmd .= ' && . "$(git --exec-path)/git-mergetool--lib"';
-		$cmd .= " && get_merge_tool_path $tool >/dev/null 2>&1";
-		$cmd .= " && can_diff >/dev/null 2>&1";
-		if (system('sh', '-c', $cmd) == 0) {
-			push(@found, $tool);
-		} else {
-			push(@notfound, $tool);
-		}
-	}
-
-	print << 'EOF';
-'git difftool --tool=<tool>' may be set to one of the following:
-EOF
-	print "\t$_\n" for (sort(@found));
+	my $cmd = 'TOOL_MODE=diff';
+	$cmd .= ' && . "$(git --exec-path)/git-mergetool--lib"';
+	$cmd .= ' && show_tool_help';
 
-	print << 'EOF';
-
-The following tools are valid, but not currently available:
-EOF
-	print "\t$_\n" for (sort(@notfound));
-
-	print << 'EOF';
-
-NOTE: Some of the tools listed above only work in a windowed
-environment. If run in a terminal-only session, they will fail.
-EOF
-	exit(0);
+	# See the comment at the bottom of file_diff() for the reason behind
+	# using system() followed by exit() instead of exec().
+	my $rc = system('sh', '-c', $cmd);
+	exit($rc | ($rc >> 8));
 }
 
 sub exit_cleanup

-- 
1.8.1.1.367.g22b1720.dirty
David Aguilar· Jan 25, 2013, 09:43 UTC · re: David Aguilar · lore

[PATCH 5/7] mergetools/vim: Remove redundant diff command

vimdiff and vimdiff2 differ only by their merge command so remove the logic in the diff command since it's not actually needed.

Signed-off-by: David Aguilar <davvid@gmail.com>
---
 mergetools/vim | 12 ++----------
 1 file changed, 2 insertions(+), 10 deletions(-)
Show changes to mergetools/vim +2 −10
diff --git a/mergetools/vim b/mergetools/vim
index 619594a..39d0327 100644
--- a/mergetools/vim
+++ b/mergetools/vim
@@ -1,14 +1,6 @@
 diff_cmd () {
-	case "$1" in
-	gvimdiff|vimdiff)
-		"$merge_tool_path" -R -f -d \
-			-c 'wincmd l' -c 'cd $GIT_PREFIX' "$LOCAL" "$REMOTE"
-		;;
-	gvimdiff2|vimdiff2)
-		"$merge_tool_path" -R -f -d \
-			-c 'wincmd l' -c 'cd $GIT_PREFIX' "$LOCAL" "$REMOTE"
-		;;
-	esac
+	"$merge_tool_path" -R -f -d \
+		-c 'wincmd l' -c 'cd $GIT_PREFIX' "$LOCAL" "$REMOTE"
 }
 
 merge_cmd () {

-- 
1.8.1.1.367.g22b1720.dirty
David Aguilar· Jan 25, 2013, 09:43 UTC · re: David Aguilar · lore

[PATCH 6/7] mergetools: Fix difftool/mergetool --tool-help listing for vim

"git difftool --tool-help" and "git mergetool --tool-help" incorreclty list "vim" as being an unavailable tool. This is because they attempt to find a tool named according to the mergetool scriptlet instead of the Git- recognized tool name.

vimdiff, vimdiff2, gvimdiff, and gvimdiff2 are all provided by the "vim" scriptlet. This required git-mergetool--lib to special-case it when setting up the tool.

Remove the exception for "vim" and allow the scriptlets to be found naturally by using symlinks to a single "vimdiff" scriptlet. This causes the --tool-help option to correctly list vimdiff, vimdiff2, gvimdiff, and gvimdiff2 in its output.

Signed-off-by: David Aguilar <davvid@gmail.com>
---
 git-mergetool--lib.sh       | 9 +--------
 mergetools/gvimdiff         | 1 +
 mergetools/gvimdiff2        | 1 +
 mergetools/{vim => vimdiff} | 0
 mergetools/vimdiff2         | 1 +
 5 files changed, 4 insertions(+), 8 deletions(-)
 create mode 120000 mergetools/gvimdiff
 create mode 120000 mergetools/gvimdiff2
 rename mergetools/{vim => vimdiff} (100%)
 create mode 120000 mergetools/vimdiff2
Show changes to 5 files +4 −8

git-mergetool--lib.sh, mergetools/gvimdiff, mergetools/gvimdiff2, mergetools/vim, mergetools/vimdiff2

diff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh
index 4c1e129..db8218a 100644
--- a/git-mergetool--lib.sh
+++ b/git-mergetool--lib.sh
@@ -44,14 +44,7 @@ valid_tool () {
 }
 
 setup_tool () {
-	case "$1" in
-	vim*|gvim*)
-		tool=vim
-		;;
-	*)
-		tool="$1"
-		;;
-	esac
+	tool="$1"
 	mergetools="$(git --exec-path)/mergetools"
 
 	# Load the default definitions
diff --git a/mergetools/gvimdiff b/mergetools/gvimdiff
new file mode 120000
index 0000000..2a72558
--- /dev/null
+++ b/mergetools/gvimdiff
@@ -0,0 +1 @@
+vimdiff
\ No newline at end of file
diff --git a/mergetools/gvimdiff2 b/mergetools/gvimdiff2
new file mode 120000
index 0000000..2a72558
--- /dev/null
+++ b/mergetools/gvimdiff2
@@ -0,0 +1 @@
+vimdiff
\ No newline at end of file
diff --git a/mergetools/vim b/mergetools/vimdiff
similarity index 100%
rename from mergetools/vim
rename to mergetools/vimdiff
diff --git a/mergetools/vimdiff2 b/mergetools/vimdiff2
new file mode 120000
index 0000000..2a72558
--- /dev/null
+++ b/mergetools/vimdiff2
@@ -0,0 +1 @@
+vimdiff
\ No newline at end of file

-- 
1.8.1.1.367.g22b1720.dirty
Sebastian Schuberth· Jan 25, 2013, 10:23 UTC · re: David Aguilar · lore

Re: [PATCH 6/7] mergetools: Fix difftool/mergetool --tool-help listing for vim

On 2013/01/25 10:43 , David Aguilar wrote:
> Remove the exception for "vim" and allow the scriptlets to be found
> naturally by using symlinks to a single "vimdiff" scriptlet.  This

I guess that won't work on platforms where Git does not support symlinks, then, like Windows. But Windows has (g)vimdiff, so loosing these on that platform would not be so nice.

-- 
Sebastian Schuberth
David Aguilar· Jan 25, 2013, 10:28 UTC · re: Sebastian Schuberth · lore

Re: [PATCH 6/7] mergetools: Fix difftool/mergetool --tool-help listing for vim

On Fri, Jan 25, 2013 at 2:23 AM, Sebastian Schuberth <sschuberth@gmail.com> wrote:

Show 9 quoted lines
> On 2013/01/25 10:43 , David Aguilar wrote:
>
>> Remove the exception for "vim" and allow the scriptlets to be found
>> naturally by using symlinks to a single "vimdiff" scriptlet.  This
>
>
> I guess that won't work on platforms where Git does not support symlinks,
> then, like Windows. But Windows has (g)vimdiff, so loosing these on that
> platform would not be so nice.
I thought Git did something sensible there like create a normal file?

If not then I was thinking we could add a provides_tools() function that each scriptlet could supply.

-- 
David
Sebastian Schuberth· Jan 25, 2013, 10:34 UTC · re: David Aguilar · lore

Re: [PATCH 6/7] mergetools: Fix difftool/mergetool --tool-help listing for vim

On Fri, Jan 25, 2013 at 11:28 AM, David Aguilar <davvid@gmail.com> wrote:
> I thought Git did something sensible there like create a normal file?

It does not. Also see my answer over here: http://stackoverflow.com/questions/11412028/git-not-storing-symlink-as-a-symlink-on-windows/11412341#11412341

> If not then I was thinking we could add a provides_tools() function that
> each scriptlet could supply.

Or maybe simply introduce scriptlets that do nothing but include / inline another scriptlet?

-- 
Sebastian Schuberth
Sebastian Schuberth· Jan 25, 2013, 11:39 UTC · re: Sebastian Schuberth · lore

Re: [PATCH 6/7] mergetools: Fix difftool/mergetool --tool-help listing for vim

On Fri, Jan 25, 2013 at 11:34 AM, Sebastian Schuberth <sschuberth@gmail.com> wrote:

>> I thought Git did something sensible there like create a normal file?
>
> It does not. Also see my answer over here:
> http://stackoverflow.com/questions/11412028/git-not-storing-symlink-as-a-symlink-on-windows/11412341#11412341

This one might be the even more appropriate answer: http://stackoverflow.com/questions/11662868/what-happens-when-i-clone-a-repository-with-symlinks-on-windows/11664406#11664406

-- 
Sebastian Schuberth
John Keeping· Jan 25, 2013, 10:38 UTC · re: David Aguilar · lore

Re: [PATCH 6/7] mergetools: Fix difftool/mergetool --tool-help listing for vim

On Fri, Jan 25, 2013 at 01:43:53AM -0800, David Aguilar wrote:
> "git difftool --tool-help" and "git mergetool --tool-help" incorreclty
> list "vim" as being an unavailable tool.  This is because they attempt
> to find a tool named according to the mergetool scriptlet instead of the Git-
> recognized tool name.

Actually, after my patches both git-difftool and git-mergetool get this right since list_merge_tool_candidates lists vimdiff and gvimdiff.

Show 6 quoted lines
> vimdiff, vimdiff2, gvimdiff, and gvimdiff2 are all provided by the "vim"
> scriptlet.  This required git-mergetool--lib to special-case it when
> setting up the tool.
> 
> Remove the exception for "vim" and allow the scriptlets to be found
> naturally by using symlinks to a single "vimdiff" scriptlet.

I wonder if it would be better to make these single-line scripts instead of symlinks:

    . "$MERGE_TOOLS_DIR"/vimdiff
where we make git-mergetool--lib.sh export:
    MERGE_TOOLS_DIR=$(git --exec-path)/mergetools
John
David Aguilar· Jan 25, 2013, 10:40 UTC · re: John Keeping · lore

Re: [PATCH 6/7] mergetools: Fix difftool/mergetool --tool-help listing for vim

On Fri, Jan 25, 2013 at 2:38 AM, John Keeping <john@keeping.me.uk> wrote:
Show 24 quoted lines
> On Fri, Jan 25, 2013 at 01:43:53AM -0800, David Aguilar wrote:
>> "git difftool --tool-help" and "git mergetool --tool-help" incorreclty
>> list "vim" as being an unavailable tool.  This is because they attempt
>> to find a tool named according to the mergetool scriptlet instead of the Git-
>> recognized tool name.
>
> Actually, after my patches both git-difftool and git-mergetool get this
> right since list_merge_tool_candidates lists vimdiff and gvimdiff.
>
>> vimdiff, vimdiff2, gvimdiff, and gvimdiff2 are all provided by the "vim"
>> scriptlet.  This required git-mergetool--lib to special-case it when
>> setting up the tool.
>>
>> Remove the exception for "vim" and allow the scriptlets to be found
>> naturally by using symlinks to a single "vimdiff" scriptlet.
>
> I wonder if it would be better to make these single-line scripts instead
> of symlinks:
>
>     . "$MERGE_TOOLS_DIR"/vimdiff
>
> where we make git-mergetool--lib.sh export:
>
>     MERGE_TOOLS_DIR=$(git --exec-path)/mergetools
That sounds like the way to go.
-- 
David
Junio C Hamano· Jan 25, 2013, 19:11 UTC · re: David Aguilar · lore

Re: [PATCH 6/7] mergetools: Fix difftool/mergetool --tool-help listing for vim

David Aguilar <davvid@gmail.com> writes:
Show 27 quoted lines
> On Fri, Jan 25, 2013 at 2:38 AM, John Keeping <john@keeping.me.uk> wrote:
>> On Fri, Jan 25, 2013 at 01:43:53AM -0800, David Aguilar wrote:
>>> "git difftool --tool-help" and "git mergetool --tool-help" incorreclty
>>> list "vim" as being an unavailable tool.  This is because they attempt
>>> to find a tool named according to the mergetool scriptlet instead of the Git-
>>> recognized tool name.
>>
>> Actually, after my patches both git-difftool and git-mergetool get this
>> right since list_merge_tool_candidates lists vimdiff and gvimdiff.
>>
>>> vimdiff, vimdiff2, gvimdiff, and gvimdiff2 are all provided by the "vim"
>>> scriptlet.  This required git-mergetool--lib to special-case it when
>>> setting up the tool.
>>>
>>> Remove the exception for "vim" and allow the scriptlets to be found
>>> naturally by using symlinks to a single "vimdiff" scriptlet.
>>
>> I wonder if it would be better to make these single-line scripts instead
>> of symlinks:
>>
>>     . "$MERGE_TOOLS_DIR"/vimdiff
>>
>> where we make git-mergetool--lib.sh export:
>>
>>     MERGE_TOOLS_DIR=$(git --exec-path)/mergetools
>
> That sounds like the way to go.

Yup, I'll expect a reroll of this one and possibly the next one (I haven't read yet).

1-5/7 looked all very sensible.
Thanks.
David Aguilar· Jan 25, 2013, 09:43 UTC · re: David Aguilar · lore

[PATCH 7/7] mergetool--lib: Improve show_tool_help() output

Check the can_diff and can_merge functions before deciding whether to add the tool to the available/unavailable lists. This makes --tool-help context- sensitive so that "git mergetool --tool-help" displays merge tools only and "git difftool --tool-help" displays diff tools only.

Signed-off-by: David Aguilar <davvid@gmail.com>
---
 git-mergetool--lib.sh | 26 +++++++++++++++++++++-----
 1 file changed, 21 insertions(+), 5 deletions(-)
Show changes to git-mergetool--lib.sh +21 −5
diff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh
index db8218a..c547c59 100644
--- a/git-mergetool--lib.sh
+++ b/git-mergetool--lib.sh
@@ -168,17 +168,33 @@ list_merge_tool_candidates () {
 }
 
 show_tool_help () {
-	list_merge_tool_candidates
 	unavailable= available= LF='
 '
-	for i in $tools
+
+	scriptlets="$(git --exec-path)"/mergetools
+	for i in "$scriptlets"/*
 	do
-		merge_tool_path=$(translate_merge_tool_path "$i")
+		. "$scriptlets"/defaults
+		. "$i"
+
+		tool="$(basename "$i")"
+		if test "$tool" = "defaults"
+		then
+			continue
+		elif merge_mode && ! can_merge
+		then
+			continue
+		elif diff_mode && ! can_diff
+		then
+			continue
+		fi
+
+		merge_tool_path=$(translate_merge_tool_path "$tool")
 		if type "$merge_tool_path" >/dev/null 2>&1
 		then
-			available="$available$i$LF"
+			available="$available$tool$LF"
 		else
-			unavailable="$unavailable$i$LF"
+			unavailable="$unavailable$tool$LF"
 		fi
 	done
 

-- 
1.8.1.1.367.g22b1720.dirty
John Keeping· Jan 25, 2013, 19:54 UTC · re: David Aguilar · lore

Re: [PATCH 7/7] mergetool--lib: Improve show_tool_help() output

On Fri, Jan 25, 2013 at 01:43:54AM -0800, David Aguilar wrote:
> Check the can_diff and can_merge functions before deciding whether to
> add the tool to the available/unavailable lists.  This makes --tool-help context-
> sensitive so that "git mergetool --tool-help" displays merge tools only
> and "git difftool --tool-help" displays diff tools only.

This log message is misleading - the existing code in list_merge_tool_candidates already filters the tools like this, so the change is more:

    mergetool--lib: don't use a hardcoded list for "--tool-help"
    Instead of using a list of tools in list_merge_tool_candidates, list
    the available scriptlets and query each of those to know whether it
    applies to diff mode and/or merge mode.
    guess_merge_tool still relies on list_merge_tool_candidates so we
    can't remove that function now.

The patch seems to do the right thing, although I have a couple of minor nits...

Show 26 quoted lines
> Signed-off-by: David Aguilar <davvid@gmail.com>
> ---
>  git-mergetool--lib.sh | 26 +++++++++++++++++++++-----
>  1 file changed, 21 insertions(+), 5 deletions(-)
> 
> diff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh
> index db8218a..c547c59 100644
> --- a/git-mergetool--lib.sh
> +++ b/git-mergetool--lib.sh
> @@ -168,17 +168,33 @@ list_merge_tool_candidates () {
>  }
>  
>  show_tool_help () {
> -	list_merge_tool_candidates
>  	unavailable= available= LF='
>  '
> -	for i in $tools
> +
> +	scriptlets="$(git --exec-path)"/mergetools
> +	for i in "$scriptlets"/*
>  	do
> -		merge_tool_path=$(translate_merge_tool_path "$i")
> +		. "$scriptlets"/defaults
> +		. "$i"
> +
> +		tool="$(basename "$i")"
Quotes are unnecessary here.
Show 10 quoted lines
> +		if test "$tool" = "defaults"
> +		then
> +			continue
> +		elif merge_mode && ! can_merge
> +		then
> +			continue
> +		elif diff_mode && ! can_diff
> +		then
> +			continue
> +		fi
Would this be better as:
    test "$tool" = "defaults" && continue
    can_merge || ! merge_mode || continue
    can_diff || ! diff_mode || continue
or is that a bit too concise?

I'd prefer to see two separate if statements either way since the "test $tool = defaults" case is different from the "does it apply to the current mode?" case. The "$tool = defaults" case could even move to the top of the loop.

Show 10 quoted lines
> +		merge_tool_path=$(translate_merge_tool_path "$tool")
>  		if type "$merge_tool_path" >/dev/null 2>&1
>  		then
> -			available="$available$i$LF"
> +			available="$available$tool$LF"
>  		else
> -			unavailable="$unavailable$i$LF"
> +			unavailable="$unavailable$tool$LF"
>  		fi
>  	done
Junio C Hamano· Jan 25, 2013, 20:06 UTC · re: John Keeping · lore

Re: [PATCH 7/7] mergetool--lib: Improve show_tool_help() output

John Keeping <john@keeping.me.uk> writes:
>> +		tool="$(basename "$i")"
>
> Quotes are unnecessary here.
Yeah, the outer quotes aren't needed; the inner ones are.
Show 19 quoted lines
>> +		if test "$tool" = "defaults"
>> +		then
>> +			continue
>> +		elif merge_mode && ! can_merge
>> +		then
>> +			continue
>> +		elif diff_mode && ! can_diff
>> +		then
>> +			continue
>> +		fi
>
> Would this be better as:
>
>     test "$tool" = "defaults" && continue
>
>     can_merge || ! merge_mode || continue
>     can_diff || ! diff_mode || continue
>
> or is that a bit too concise?

It is beyond "too concise"; it is unreadable, and more importantly, the latter two lines are illogical (why do you even ask if it can be used for merging, before asking merge_mode to see if the answer to that question matters to you?)

John Keeping· Jan 25, 2013, 20:08 UTC · re: John Keeping · lore

Re: [PATCH 7/7] mergetool--lib: Improve show_tool_help() output

On Fri, Jan 25, 2013 at 07:54:46PM +0000, John Keeping wrote:
Show 78 quoted lines
> On Fri, Jan 25, 2013 at 01:43:54AM -0800, David Aguilar wrote:
> > Check the can_diff and can_merge functions before deciding whether to
> > add the tool to the available/unavailable lists.  This makes --tool-help context-
> > sensitive so that "git mergetool --tool-help" displays merge tools only
> > and "git difftool --tool-help" displays diff tools only.
> 
> This log message is misleading - the existing code in
> list_merge_tool_candidates already filters the tools like this, so the
> change is more:
> 
>     mergetool--lib: don't use a hardcoded list for "--tool-help"
> 
>     Instead of using a list of tools in list_merge_tool_candidates, list
>     the available scriptlets and query each of those to know whether it
>     applies to diff mode and/or merge mode.
> 
>     guess_merge_tool still relies on list_merge_tool_candidates so we
>     can't remove that function now.
> 
> 
> The patch seems to do the right thing, although I have a couple of minor
> nits...
> 
> > Signed-off-by: David Aguilar <davvid@gmail.com>
> > ---
> >  git-mergetool--lib.sh | 26 +++++++++++++++++++++-----
> >  1 file changed, 21 insertions(+), 5 deletions(-)
> > 
> > diff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh
> > index db8218a..c547c59 100644
> > --- a/git-mergetool--lib.sh
> > +++ b/git-mergetool--lib.sh
> > @@ -168,17 +168,33 @@ list_merge_tool_candidates () {
> >  }
> >  
> >  show_tool_help () {
> > -	list_merge_tool_candidates
> >  	unavailable= available= LF='
> >  '
> > -	for i in $tools
> > +
> > +	scriptlets="$(git --exec-path)"/mergetools
> > +	for i in "$scriptlets"/*
> >  	do
> > -		merge_tool_path=$(translate_merge_tool_path "$i")
> > +		. "$scriptlets"/defaults
> > +		. "$i"
> > +
> > +		tool="$(basename "$i")"
> 
> Quotes are unnecessary here.
> 
> > +		if test "$tool" = "defaults"
> > +		then
> > +			continue
> > +		elif merge_mode && ! can_merge
> > +		then
> > +			continue
> > +		elif diff_mode && ! can_diff
> > +		then
> > +			continue
> > +		fi
> 
> Would this be better as:
> 
>     test "$tool" = "defaults" && continue
> 
>     can_merge || ! merge_mode || continue
>     can_diff || ! diff_mode || continue
> 
> or is that a bit too concise?
> 
> I'd prefer to see two separate if statements either way since the "test
> $tool = defaults" case is different from the "does it apply to the
> current mode?" case.  The "$tool = defaults" case could even move to the
> top of the loop.
> 
> > +		merge_tool_path=$(translate_merge_tool_path "$tool")
Actually, can we just change all of the above part of the loop to:
	test "$tool" = defaults && continue
	merge_tool_path=$(
		setup_tool "$tool" >/dev/null 2>&1 &&
		translate_merge_tool_path "$tool"
	) || continue
Show 13 quoted lines
> >  		if type "$merge_tool_path" >/dev/null 2>&1
> >  		then
> > -			available="$available$i$LF"
> > +			available="$available$tool$LF"
> >  		else
> > -			unavailable="$unavailable$i$LF"
> > +			unavailable="$unavailable$tool$LF"
> >  		fi
> >  	done
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
Junio C Hamano· Jan 25, 2013, 20:16 UTC · re: John Keeping · lore

Re: [PATCH 7/7] mergetool--lib: Improve show_tool_help() output

John Keeping <john@keeping.me.uk> writes:
Show 8 quoted lines
> Actually, can we just change all of the above part of the loop to:
>
> 	test "$tool" = defaults && continue
>
> 	merge_tool_path=$(
> 		setup_tool "$tool" >/dev/null 2>&1 &&
> 		translate_merge_tool_path "$tool"
> 	) || continue

Meaning "setup_tool ought to know which mode we are in and should fail if we are in merge mode and it does not support merging"? That line of reasoning makes tons of sense to me, compared to this script implementing that logic for these scriptlets.

How/when does translate_merge_tool_path fail?
Show 14 quoted lines
>
>> >  		if type "$merge_tool_path" >/dev/null 2>&1
>> >  		then
>> > -			available="$available$i$LF"
>> > +			available="$available$tool$LF"
>> >  		else
>> > -			unavailable="$unavailable$i$LF"
>> > +			unavailable="$unavailable$tool$LF"
>> >  		fi
>> >  	done
>> --
>> To unsubscribe from this list: send the line "unsubscribe git" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
John Keeping· Jan 25, 2013, 20:46 UTC · re: Junio C Hamano · lore

Re: [PATCH 7/7] mergetool--lib: Improve show_tool_help() output

On Fri, Jan 25, 2013 at 12:16:42PM -0800, Junio C Hamano wrote:
Show 15 quoted lines
> John Keeping <john@keeping.me.uk> writes:
> 
> > Actually, can we just change all of the above part of the loop to:
> >
> > 	test "$tool" = defaults && continue
> >
> > 	merge_tool_path=$(
> > 		setup_tool "$tool" >/dev/null 2>&1 &&
> > 		translate_merge_tool_path "$tool"
> > 	) || continue
> 
> Meaning "setup_tool ought to know which mode we are in and should
> fail if we are in merge mode and it does not support merging"?  That
> line of reasoning makes tons of sense to me, compared to this script
> implementing that logic for these scriptlets.

Yes, that's part of what setup_tool does. It actually calls "exit" if the "mode? && can_mode" test fails, which is why we need to call it in the subshell.

I think this would get even better if we add a preparatory patch like this, so we can just call setup_tool and then set merge_tool_path:

-- >8 --
Show changes to git-mergetool--lib.sh +3 −4
diff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh
index 888ae3e..4644cbf 100644
--- a/git-mergetool--lib.sh
+++ b/git-mergetool--lib.sh
@@ -67,11 +67,11 @@ setup_tool () {
 	if merge_mode && ! can_merge
 	then
 		echo "error: '$tool' can not be used to resolve merges" >&2
-		exit 1
+		return 1
 	elif diff_mode && ! can_diff
 	then
 		echo "error: '$tool' can only be used to resolve merges" >&2
-		exit 1
+		return 1
 	fi
 	return 0
 }
@@ -100,7 +100,7 @@ run_merge_tool () {
 	status=0
 
 	# Bring tool-specific functions into scope
-	setup_tool "$1"
+	setup_tool "$1" || return
 
 	if merge_mode
 	then

-- 8< --
 
> How/when does translate_merge_tool_path fail?

It doesn't - the "|| continue" is to catch errors from setup_tool.


John
Junio C Hamano· Jan 25, 2013, 20:56 UTC · re: John Keeping · lore

Re: [PATCH 7/7] mergetool--lib: Improve show_tool_help() output

John Keeping <john@keeping.me.uk> writes:
Show 59 quoted lines
> On Fri, Jan 25, 2013 at 12:16:42PM -0800, Junio C Hamano wrote:
>> John Keeping <john@keeping.me.uk> writes:
>> 
>> > Actually, can we just change all of the above part of the loop to:
>> >
>> > 	test "$tool" = defaults && continue
>> >
>> > 	merge_tool_path=$(
>> > 		setup_tool "$tool" >/dev/null 2>&1 &&
>> > 		translate_merge_tool_path "$tool"
>> > 	) || continue
>> 
>> Meaning "setup_tool ought to know which mode we are in and should
>> fail if we are in merge mode and it does not support merging"?  That
>> line of reasoning makes tons of sense to me, compared to this script
>> implementing that logic for these scriptlets.
>
> Yes, that's part of what setup_tool does.  It actually calls "exit" if
> the "mode? && can_mode" test fails, which is why we need to call it in
> the subshell.
>
> I think this would get even better if we add a preparatory patch like
> this, so we can just call setup_tool and then set merge_tool_path:
>
> -- >8 --
>
> diff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh
> index 888ae3e..4644cbf 100644
> --- a/git-mergetool--lib.sh
> +++ b/git-mergetool--lib.sh
> @@ -67,11 +67,11 @@ setup_tool () {
>  	if merge_mode && ! can_merge
>  	then
>  		echo "error: '$tool' can not be used to resolve merges" >&2
> -		exit 1
> +		return 1
>  	elif diff_mode && ! can_diff
>  	then
>  		echo "error: '$tool' can only be used to resolve merges" >&2
> -		exit 1
> +		return 1
>  	fi
>  	return 0
>  }
> @@ -100,7 +100,7 @@ run_merge_tool () {
>  	status=0
>  
>  	# Bring tool-specific functions into scope
> -	setup_tool "$1"
> +	setup_tool "$1" || return
>  
>  	if merge_mode
>  	then
>
> -- 8< --
>  
>> How/when does translate_merge_tool_path fail?
>
> It doesn't - the "|| continue" is to catch errors from setup_tool.
Ugh.
John Keeping· Jan 25, 2013, 21:16 UTC · re: Junio C Hamano · lore

Re: [PATCH 7/7] mergetool--lib: Improve show_tool_help() output

On Fri, Jan 25, 2013 at 12:56:37PM -0800, Junio C Hamano wrote:
Show 63 quoted lines
> John Keeping <john@keeping.me.uk> writes:
> 
> > On Fri, Jan 25, 2013 at 12:16:42PM -0800, Junio C Hamano wrote:
> >> John Keeping <john@keeping.me.uk> writes:
> >> 
> >> > Actually, can we just change all of the above part of the loop to:
> >> >
> >> > 	test "$tool" = defaults && continue
> >> >
> >> > 	merge_tool_path=$(
> >> > 		setup_tool "$tool" >/dev/null 2>&1 &&
> >> > 		translate_merge_tool_path "$tool"
> >> > 	) || continue
> >> 
> >> Meaning "setup_tool ought to know which mode we are in and should
> >> fail if we are in merge mode and it does not support merging"?  That
> >> line of reasoning makes tons of sense to me, compared to this script
> >> implementing that logic for these scriptlets.
> >
> > Yes, that's part of what setup_tool does.  It actually calls "exit" if
> > the "mode? && can_mode" test fails, which is why we need to call it in
> > the subshell.
> >
> > I think this would get even better if we add a preparatory patch like
> > this, so we can just call setup_tool and then set merge_tool_path:
> >
> > -- >8 --
> >
> > diff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh
> > index 888ae3e..4644cbf 100644
> > --- a/git-mergetool--lib.sh
> > +++ b/git-mergetool--lib.sh
> > @@ -67,11 +67,11 @@ setup_tool () {
> >  	if merge_mode && ! can_merge
> >  	then
> >  		echo "error: '$tool' can not be used to resolve merges" >&2
> > -		exit 1
> > +		return 1
> >  	elif diff_mode && ! can_diff
> >  	then
> >  		echo "error: '$tool' can only be used to resolve merges" >&2
> > -		exit 1
> > +		return 1
> >  	fi
> >  	return 0
> >  }
> > @@ -100,7 +100,7 @@ run_merge_tool () {
> >  	status=0
> >  
> >  	# Bring tool-specific functions into scope
> > -	setup_tool "$1"
> > +	setup_tool "$1" || return
> >  
> >  	if merge_mode
> >  	then
> >
> > -- 8< --
> >  
> >> How/when does translate_merge_tool_path fail?
> >
> > It doesn't - the "|| continue" is to catch errors from setup_tool.
> 
> Ugh.

Is that targeted at my suggestion at the top of this email or calling exit in setup_tool?

With the patch above, the block of code at the top becomes:
 	test "$tool" = defaults && continue
 	setup_tool "$tool" 2>/dev/null || continue
 	merge_tool_path=$(translate_merge_tool_path "$tool")
which IMHO is pretty readable.
John
Junio C Hamano· Jan 25, 2013, 21:47 UTC · re: John Keeping · lore

Re: [PATCH 7/7] mergetool--lib: Improve show_tool_help() output

John Keeping <john@keeping.me.uk> writes:
Show 6 quoted lines
>> > It doesn't - the "|| continue" is to catch errors from setup_tool.
>> 
>> Ugh.
>
> Is that targeted at my suggestion at the top of this email or calling
> exit in setup_tool?

At the fact that you had to go a convoluted route because you cannot just run setup_tool in subshell and do translate_merge_tool_path after that, because the latter needs to look at the shell variable the former sets.

Show 8 quoted lines
> With the patch above, the block of code at the top becomes:
>
>  	test "$tool" = defaults && continue
>
>  	setup_tool "$tool" 2>/dev/null || continue
>  	merge_tool_path=$(translate_merge_tool_path "$tool")
>
> which IMHO is pretty readable.

Of course it is. The current callers of setup_tool may need some adjustments, but that should be fairly trivial, I hope.

John Keeping· Jan 25, 2013, 22:02 UTC · re: Junio C Hamano · lore

Re: [PATCH 7/7] mergetool--lib: Improve show_tool_help() output

On Fri, Jan 25, 2013 at 01:47:59PM -0800, Junio C Hamano wrote:
Show 12 quoted lines
> John Keeping <john@keeping.me.uk> writes:
> > With the patch above, the block of code at the top becomes:
> >
> >  	test "$tool" = defaults && continue
> >
> >  	setup_tool "$tool" 2>/dev/null || continue
> >  	merge_tool_path=$(translate_merge_tool_path "$tool")
> >
> > which IMHO is pretty readable.
> 
> Of course it is.  The current callers of setup_tool may need some
> adjustments, but that should be fairly trivial, I hope.

There are only two and one of them already seems like it doesn't want the command to cause the script to exit.

David, can you incorporate the following two patches when you re-roll? Your original 7/7 with the change above will want to build on 8/7.

John
John Keeping· Jan 25, 2013, 22:03 UTC · re: John Keeping · lore

[PATCH 8/7] mergetool--lib: don't call "exit" in setup_tool

This will make it easier to use setup_tool in places where we expect that the selected tool will not support the current mode.

Signed-off-by: John Keeping <john@keeping.me.uk>
---
 git-mergetool--lib.sh | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)
Show changes to git-mergetool--lib.sh +3 −3
diff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh
index 4c1e129..c6bd8ba 100644
--- a/git-mergetool--lib.sh
+++ b/git-mergetool--lib.sh
@@ -67,11 +67,11 @@ setup_tool () {
 	if merge_mode && ! can_merge
 	then
 		echo "error: '$tool' can not be used to resolve merges" >&2
-		exit 1
+		return 1
 	elif diff_mode && ! can_diff
 	then
 		echo "error: '$tool' can only be used to resolve merges" >&2
-		exit 1
+		return 1
 	fi
 	return 0
 }
@@ -100,7 +100,7 @@ run_merge_tool () {
 	status=0
 
 	# Bring tool-specific functions into scope
-	setup_tool "$1"
+	setup_tool "$1" || return
 
 	if merge_mode
 	then

-- 
1.8.1
Junio C Hamano· Jan 26, 2013, 00:24 UTC · re: John Keeping · lore

Re: [PATCH 8/7] mergetool--lib: don't call "exit" in setup_tool

Applying this one on top of 1/7 thru 5/7 and 7/7 seems to break t7610 rather badly.

--- >8 ------ >8 ------ >8 ------ >8 ------ >8 ------ >8 --- ... ok 1 - setup

expecting success:
    git checkout -b test1 branch1 &&
    git submodule update -N &&
    test_must_fail git merge master >/dev/null 2>&1 &&
    ( yes "" | git mergetool both >/dev/null 2>&1 ) &&
    ( 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 ) &&
    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"

M submod Switched to a new branch 'test1' Submodule path 'submod': checked out '39c7f044ed2e6a9cebd5266529badd181c8762b5' not ok - 2 custom mergetool # # git checkout -b test1 branch1 && # git submodule update -N && # test_must_fail git merge master >/dev/null 2>&1 && # ( yes "" | git mergetool both >/dev/null 2>&1 ) && # ( 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 ) && # 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" # --- 8< ------ 8< ------ 8< ------ 8< ------ 8< ------ 8< ---

Due to ">dev/null 2>&1", all of the error clues are hidden, and I didn't dig further to see which one was failing (this is why tests shouldn't do these in general).

David Aguilar· Jan 26, 2013, 07:01 UTC · re: Junio C Hamano · lore

Re: [PATCH 8/7] mergetool--lib: don't call "exit" in setup_tool

On Fri, Jan 25, 2013 at 4:24 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Applying this one on top of 1/7 thru 5/7 and 7/7 seems to break
> t7610 rather badly.

I just sent a replacement for the vim/symlink issue stuff. I tried to keep the patch small. John, can you rebase this patch on top of it?

Show 47 quoted lines
> --- >8 ------ >8 ------ >8 ------ >8 ------ >8 ------ >8 ---
> ...
> ok 1 - setup
>
> expecting success:
>     git checkout -b test1 branch1 &&
>     git submodule update -N &&
>     test_must_fail git merge master >/dev/null 2>&1 &&
>     ( yes "" | git mergetool both >/dev/null 2>&1 ) &&
>     ( 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 ) &&
>     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"
>
> M       submod
> Switched to a new branch 'test1'
> Submodule path 'submod': checked out '39c7f044ed2e6a9cebd5266529badd181c8762b5'
> not ok - 2 custom mergetool
> #
> #           git checkout -b test1 branch1 &&
> #           git submodule update -N &&
> #           test_must_fail git merge master >/dev/null 2>&1 &&
> #           ( yes "" | git mergetool both >/dev/null 2>&1 ) &&
> #           ( 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 ) &&
> #           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"
> #
> --- 8< ------ 8< ------ 8< ------ 8< ------ 8< ------ 8< ---
>
> Due to ">dev/null 2>&1", all of the error clues are hidden, and I
> didn't dig further to see which one was failing (this is why tests
> shouldn't do these in general).
-- 
David
John Keeping· Jan 26, 2013, 12:17 UTC · re: Junio C Hamano · lore

[PATCH 1/2 v2] mergetool--lib: don't call "exit" in setup_tool

This will make it easier to use setup_tool in places where we expect that the selected tool will not support the current mode.

Signed-off-by: John Keeping <john@keeping.me.uk>
---
On Fri, Jan 25, 2013 at 04:24:03PM -0800, Junio C Hamano wrote:
> Applying this one on top of 1/7 thru 5/7 and 7/7 seems to break
> t7610 rather badly.

Sorry about that. The 'setup_tool' function should really be called 'setup_builtin_tool' - it isn't necessary when a custom mergetool is configured and will return 1 when called with an argument that isn't a builtin tool from $GIT_EXEC_PATH/mergetools.

The change is the second hunk below which now wraps the call to setup_tool in an if block as well as adding the "|| return".

 git-mergetool--lib.sh | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)
Show changes to git-mergetool--lib.sh +6 −3
diff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh
index 4c1e129..8a5eaff 100644
--- a/git-mergetool--lib.sh
+++ b/git-mergetool--lib.sh
@@ -67,11 +67,11 @@ setup_tool () {
 	if merge_mode && ! can_merge
 	then
 		echo "error: '$tool' can not be used to resolve merges" >&2
-		exit 1
+		return 1
 	elif diff_mode && ! can_diff
 	then
 		echo "error: '$tool' can only be used to resolve merges" >&2
-		exit 1
+		return 1
 	fi
 	return 0
 }
@@ -100,7 +100,10 @@ run_merge_tool () {
 	status=0
 
 	# Bring tool-specific functions into scope
-	setup_tool "$1"
+	if test -z "$merge_tool_path"
+	then
+		setup_tool "$1" || return
+	fi
 
 	if merge_mode
 	then

-- 
1.8.1.1.367.ga9c3dd4.dirty
John Keeping· Jan 26, 2013, 13:50 UTC · re: John Keeping · lore

[PATCH 1/2 v3] mergetool--lib: don't call "exit" in setup_tool

This will make it easier to use setup_tool in places where we expect that the selected tool will not support the current mode.

We need to introduce a new return code for setup_tool to differentiate between the case of "the selected tool is invalid" and "the selected tool is not a built-in" since we must call setup_tool when a custom 'merge.<tool>.path' is configured for a built-in tool but avoid failing when the configured tool is not a built-in.

Signed-off-by: John Keeping <john@keeping.me.uk>
---
Show 11 quoted lines
> On Fri, Jan 25, 2013 at 04:24:03PM -0800, Junio C Hamano wrote:
> > Applying this one on top of 1/7 thru 5/7 and 7/7 seems to break
> > t7610 rather badly.
> 
> Sorry about that.  The 'setup_tool' function should really be called
> 'setup_builtin_tool' - it isn't necessary when a custom mergetool is
> configured and will return 1 when called with an argument that isn't a
> builtin tool from $GIT_EXEC_PATH/mergetools.
> 
> The change is the second hunk below which now wraps the call to
> setup_tool in an if block as well as adding the "|| return".

Now that I've run the entire test suite, that still wasn't correct since it did not correctly handle the case where the user overrides the path for one of the built-in mergetools.

 git-mergetool--lib.sh | 23 ++++++++++++++++++++---
 1 file changed, 20 insertions(+), 3 deletions(-)
Show changes to git-mergetool--lib.sh +20 −3
diff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh
index 4c1e129..dd4f088 100644
--- a/git-mergetool--lib.sh
+++ b/git-mergetool--lib.sh
@@ -58,7 +58,11 @@ setup_tool () {
 	. "$mergetools/defaults"
 	if ! test -f "$mergetools/$tool"
 	then
-		return 1
+		# Use a special return code for this case since we want to
+		# source "defaults" even when an explicit tool path is
+		# configured since the user can use that to override the
+		# default path in the scriptlet.
+		return 2
 	fi
 
 	# Load the redefined functions
@@ -67,11 +71,11 @@ setup_tool () {
 	if merge_mode && ! can_merge
 	then
 		echo "error: '$tool' can not be used to resolve merges" >&2
-		exit 1
+		return 1
 	elif diff_mode && ! can_diff
 	then
 		echo "error: '$tool' can only be used to resolve merges" >&2
-		exit 1
+		return 1
 	fi
 	return 0
 }
@@ -101,6 +105,19 @@ run_merge_tool () {
 
 	# Bring tool-specific functions into scope
 	setup_tool "$1"
+	exitcode=$?
+	case $exitcode in
+	0)
+		:
+		;;
+	2)
+		# The configured tool is not a built-in tool.
+		test -n "$merge_tool_path" || return 1
+		;;
+	*)
+		return $exitcode
+		;;
+	esac
 
 	if merge_mode
 	then

-- 
1.8.1
John Keeping· Jan 25, 2013, 22:04 UTC · re: John Keeping · lore

[PATCH 9/7] mergetool--lib: fix path lookup in guess_merge_tool

guess_merge_tool calls translate_merge_tool_path in order to get the correct name of the tool to check whether it can be found on the user's system. But this function is designed to be overridden by tool scriptlets so it does nothing if the relevant scriptlet has not been sourced.

Fix this by calling setup_tool before doing anything.
Signed-off-by: John Keeping <john@keeping.me.uk>
---
 git-mergetool--lib.sh | 1 +
 1 file changed, 1 insertion(+)
Show changes to git-mergetool--lib.sh +1 −0
diff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh
index c6bd8ba..46860c5 100644
--- a/git-mergetool--lib.sh
+++ b/git-mergetool--lib.sh
@@ -219,6 +219,7 @@ guess_merge_tool () {
 	# Loop over each candidate and stop when a valid merge tool is found.
 	for i in $tools
 	do
+		setup_tool "$i" 2>&1 || continue
 		merge_tool_path="$(translate_merge_tool_path "$i")"
 		if type "$merge_tool_path" >/dev/null 2>&1
 		then

-- 
1.8.1

← back to recent threads